RFR JDK-8007609

[John Zavgren]

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.


-- 
John Zavgren
john.zavgren at oracle.com
603-821-0904
US-Burlington-MA