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
