fixes i#695 uninit read

Reviewer: bruening@google.com
fixes i#695 uninit read

    fixes issue 695
    - read uninit buffer.

[bruening, 2012-03-06 21:49:23]

http://codereview.appspot.com/5757054/diff/1/core/win32/os.c
File core/win32/os.c (right):

http://codereview.appspot.com/5757054/diff/1/core/win32/os.c#newcode4562
core/win32/os.c:4562: ASSERT(buf != NULL && buf_len != 0);
this should be at the top of the function (some paths return up above so not as
effective way down here)

http://codereview.appspot.com/5757054/diff/1/core/win32/os.c#newcode4566
core/win32/os.c:4566: buf[size] = L'\0';
this is unnecessary: snwprintf guarantees to write this

http://codereview.appspot.com/5757054/diff/1/core/win32/os.c#newcode4568
core/win32/os.c:4568: buf[buf_len-1] = L'\0';
on error perhaps should return false since it is a user-supplied buffer and not
this routine's fault if it's too small

[bruening, 2012-03-07 00:54:08]

http://codereview.appspot.com/5757054/diff/3001/core/win32/os.c
File core/win32/os.c (right):

http://codereview.appspot.com/5757054/diff/3001/core/win32/os.c#newcode4564
core/win32/os.c:4564: is_UNC ? L"UNC" : L"", name);
this makes me nervous and seems fragile: best practice is to always
null-terminate right after the snwprintf.  someone might add code in between
here and the for loop and get into trouble in the future.

http://codereview.appspot.com/5757054/diff/3001/core/win32/os.c#newcode4567
core/win32/os.c:4567: if (L'\0'== buf[i])
missing space, and harder to read this way (I know you're matching the one
below): more natural to read with buf[i] first.  people starting writing the
const first to try and avoid "=" vs "==" bugs, but today's compilers all catch
that, and it's not worth the reduction in readability.

but, IMHO better to get return value of snwprintf, fail if error, o/w loop to
returned count: seems much more natural and efficient

http://codereview.appspot.com/5757054/diff/3001/core/win32/os.c#newcode4572
core/win32/os.c:4572: if (i == buf_len) {
IMHO the code is much more natural and easier to read to just check the return
value of snwprintf and return false in that case

[adam2, 2012-03-07 02:16:54]

http://codereview.appspot.com/5757054/diff/3001/core/win32/os.c
File core/win32/os.c (right):

http://codereview.appspot.com/5757054/diff/3001/core/win32/os.c#newcode4564
core/win32/os.c:4564: is_UNC ? L"UNC" : L"", name);
On 2012/03/07 00:54:08, bruening wrote:
> this makes me nervous and seems fragile: best practice is to always
> null-terminate right after the snwprintf.  someone might add code in between
> here and the for loop and get into trouble in the future.

Done.

http://codereview.appspot.com/5757054/diff/3001/core/win32/os.c#newcode4567
core/win32/os.c:4567: if (L'\0'== buf[i])
On 2012/03/07 00:54:08, bruening wrote:
> missing space, and harder to read this way (I know you're matching the one
> below): more natural to read with buf[i] first.  people starting writing the
> const first to try and avoid "=" vs "==" bugs, but today's compilers all catch
> that, and it's not worth the reduction in readability.
> 
> but, IMHO better to get return value of snwprintf, fail if error, o/w loop to
> returned count: seems much more natural and efficient

Done.

http://codereview.appspot.com/5757054/diff/3001/core/win32/os.c#newcode4572
core/win32/os.c:4572: if (i == buf_len) {
On 2012/03/07 00:54:08, bruening wrote:
> IMHO the code is much more natural and easier to read to just check the return
> value of snwprintf and return false in that case

Done.

[bruening, 2012-03-07 16:25:24]

not sure why you keep putting in the every-iter check for null: once removed in
favor of proper loop bound, LGTM

http://codereview.appspot.com/5757054/diff/7001/core/win32/os.c
File core/win32/os.c (right):

http://codereview.appspot.com/5757054/diff/7001/core/win32/os.c#newcode4484
core/win32/os.c:4484: int  res;
has superfluous space

http://codereview.appspot.com/5757054/diff/7001/core/win32/os.c#newcode4571
core/win32/os.c:4571: if (buf[i] == L'\0')
wasteful: just change loop bound to res and avoid this check altogether