RFR JDK-8007609

[David Holmes]

John,

I think the functional fix is okay but you have obscured it in so much 
"cleanup" that it is hard to say with 100% certainty. Please leave 
extensive cleanups to separate bugs - in this case I'm not seeing 
improvements in readability in a number of places (indentation is odd) 
and in at least one case you have made a change that violates the coding 
guidelines you cite eg:

  126             } else
  127                 len = 0;

The else should have been left as a block.

  121             } else {
  122                 len = 0;
  123             }

David
-----

On 9/02/2013 3:03 AM, John Zavgren wrote:
> Greetings:
> I posted a new webrev image:
> http://cr.openjdk.java.net/~jzavgren/8007609/webrev.03/
> <http://cr.openjdk.java.net/%7Ejzavgren/8007609/webrev.03/>
>
> The sole "functional" revision is contained in the following small code
> snippet:
>
>     -            /* retry with a buffer of the right size */
>     -            result = (WCHAR*)realloc(result, (len+1) * sizeof(WCHAR));
>     -            if (result != NULL) {
>     -                len = (*GetFinalPathNameByHandle_func)(h, result,
> len, 0);
>     -            } else {
>     +            /* retry the procedure with a buffer of the right size. */
>     +            WCHAR * newResult  = (WCHAR*)realloc(result, (len+1) *
> sizeof(WCHAR));
>     +            if (newResult != NULL) {
>     +                result = newResult;
>     +                len = (*GetFinalPathNameByHandle_func)(h,
> newResult, len, 0);
>     +            } else
>
> and, the innovation is the use of a local variable to hold the attempted
> memory reallocation. This makes the code simpler and easier to understand.
>
> I introduced a huge number of additional changes in the file that are my
> attempt to make the file consistent with our style guidelines.
>
> Changes include:
> 1.) elimination of tab characters.
> 2.) spelling, punctuation,  and grammar corrections in the comments.
> 3.) truncation of lines that exceed 80 characters
> 4.) correction of indentation, line wrapping, etc.
>
> I hope I haven't missed anything.
> http://www.oracle.com/technetwork/java/javase/documentation/codeconvtoc-136057.html
>
>
> On 02/08/2013 05:45 AM, Chris Hegarty wrote:
>> Apologies, you are correct. I'll book an appointment with the optician!
>>
>> -Chris.
>>
>> On 08/02/2013 00:15, David Holmes wrote:
>>> On 7/02/2013 10:54 PM, Chris Hegarty wrote:
>>>> On 02/07/2013 11:54 AM, David Holmes wrote:
>>>>> ....
>>>>>> AFAICS setting len=0 means len==0 will be true and so we will
>>>>>> free(result).
>>>>>
>>>>> And if len != 0 then we will have already freed result, so avoiding a
>>>>> double-free.
>>>>
>>>> Here's the code as it stands today.
>>>
>>> Yes .... I don't see the problem
>>>
>>>>
>>>> 113 result = (WCHAR*)malloc(MAX_PATH * sizeof(WCHAR));
>>>> 114 if (result != NULL) {
>>>
>>> we've entered this block so we must free result evetually.
>>>
>>>> 115 DWORD len = (*GetFinalPathNameByHandle_func)(h, result,
>>>> MAX_PATH, 0);
>>>> 116 if (len >= MAX_PATH) {
>>>> 117 /* retry with a buffer of the right size */
>>>> 118 result = (WCHAR*)realloc(result, (len+1) * sizeof(WCHAR));
>>>> 119 if (result != NULL) {
>>>> 120 len = (*GetFinalPathNameByHandle_func)(h, result, len, 0);
>>>> 121 } else {
>>>> 122 len = 0;
>>>> 123 }
>>>> 124 }
>>>> 125 if (len > 0) {
>>>
>>> len was good so we've gone this path
>>>
>>>> 126 /**
>>>> 127 * Strip prefix (should be \\?\ or \\?\UNC)
>>>> 128 */
>>>> 129 if (result[0] == L'\\' && result[1] == L'\\' &&
>>>> 130 result[2] == L'?' && result[3] == L'\\')
>>>> 131 {
>>>> 132 int isUnc = (result[4] == L'U' &&
>>>> 133 result[5] == L'N' &&
>>>> 134 result[6] == L'C');
>>>> 135 int prefixLen = (isUnc) ? 7 : 4;
>>>> 136 /* actual result length (includes terminator) */
>>>> 137 int resultLen = len - prefixLen + (isUnc ? 1 : 0) + 1;
>>>> 138
>>>> 139 /* copy result without prefix into new buffer */
>>>> 140 WCHAR *tmp = (WCHAR*)malloc(resultLen * sizeof(WCHAR));
>>>> 141 if (tmp == NULL) {
>>>> 142 len = 0; <<<<<<<<<<<<<<<<<<< HERE
>>>
>>> malloc failed so we need to bail out. We will now skip to line 161
>>>
>>>> 143 } else {
>>>> 144 WCHAR *p = result;
>>>> 145 p += prefixLen;
>>>> 146 if (isUnc) {
>>>> 147 WCHAR *p2 = tmp;
>>>> 148 p2[0] = L'\\';
>>>> 149 p2++;
>>>> 150 wcscpy(p2, p);
>>>> 151 } else {
>>>> 152 wcscpy(tmp, p);
>>>> 153 }
>>>> 154 free(result);
>>>> 155 result = tmp;
>>>> 156 }
>>>> 157 }
>>>> 158 }
>>>> 159
>>>> 160 /* unable to get final path */
>>>> 161 if (len == 0 && result != NULL) {
>>>
>>> We reach here because len==0 and result != NULL
>>>
>>>> 162 free(result);
>>>> 163 result = NULL;
>>>> 164 }
>>>> 165 }
>>>
>>> Looks fine to me.
>>> David
>>>
>>>> -Chris.
>
>