RFR JDK-8007609

[David Holmes]

Looks fine to me.

Thanks,
David

On 19/02/2013 2:13 AM, John Zavgren wrote:
> 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
>