RFR JDK-8007609

[John Zavgren]

On 02/11/2013 08:33 PM, David Holmes wrote:
> 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.
>>
>>
Greetings:
I just posted a new webrev image:

http://cr.openjdk.java.net/~jzavgren/8007609/webrev.05/ 
<http://cr.openjdk.java.net/%7Ejzavgren/8007609/webrev.05/>

The change is now focused on fixing the memory allocation issue... all 
the formatting changes have been removed.

Thanks!
John

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