RFR JDK-8007609 (original) (raw)

David Holmes david.holmes at oracle.com
Tue Feb 12 01:33:16 UTC 2013

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 = (*GetFinalPathNameByHandlefunc)(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 = (*GetFinalPathNameByHandlefunc)(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(MAXPATH * sizeof(WCHAR)); 114 if (result != NULL) { we've entered this block so we must free result evetually. 115 DWORD len = (*GetFinalPathNameByHandlefunc)(h, result, MAXPATH, 0); 116 if (len >= MAXPATH) { 117 /* retry with a buffer of the right size */ 118 result = (WCHAR*)realloc(result, (len+1) * sizeof(WCHAR)); 119 if (result != NULL) { 120 len = (*GetFinalPathNameByHandlefunc)(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.

More information about the core-libs-dev mailing list