NaCl: Fix off-by-one in getcwd() syscall emulation.

NaCl: Fix off-by-one in getcwd() syscall emulation.

TEST= manual testing on OSX + toolchain trybots
BUG= https://code.google.com/p/nativeclient/issues/detail?id=4010
R=mcgrathr@chromium.org

Committed: https://git.chromium.org/gitweb?p=native_client/nacl-glibc.git;a=commit;h=1106743ae743989ed845d2bf5156535fbf2c7383

[Sam Clegg, 2014-12-06 18:05:17]

sbc@chromium.org changed reviewers:
+ mcgrathr@chromium.org

[Roland McGrath, 2014-12-08 20:56:48]

https://codereview.chromium.org/759833008/diff/1/sysdeps/nacl/sysdep.h
File sysdeps/nacl/sysdep.h (right):

https://codereview.chromium.org/759833008/diff/1/sysdeps/nacl/sysdep.h#newcod...
sysdeps/nacl/sysdep.h:601: for (len = 0; buf[len] != '\0'; ++len);
Why is it safe to presume it's terminated and not check size?
Either way, this should just call strlen or __strnlen rather than using a loop.

[Sam Clegg, 2014-12-08 21:10:29]

https://codereview.chromium.org/759833008/diff/1/sysdeps/nacl/sysdep.h
File sysdeps/nacl/sysdep.h (right):

https://codereview.chromium.org/759833008/diff/1/sysdeps/nacl/sysdep.h#newcod...
sysdeps/nacl/sysdep.h:601: for (len = 0; buf[len] != '\0'; ++len);
On 2014/12/08 20:56:48, Roland McGrath wrote:
> Why is it safe to presume it's terminated and not check size?
> Either way, this should just call strlen or __strnlen rather than using a
loop.

Because the __nacl_irt_getcwd will fail if it couldn't fit the enture
null terminiated string in buf.  So we could have returned already
above in this case.

I will try strlen.  I was assuming whoever wrote this has a good reason
for using a loop.

[Roland McGrath, 2014-12-08 21:16:51]

On 2014/12/08 21:10:29, Sam Clegg wrote:
> https://codereview.chromium.org/759833008/diff/1/sysdeps/nacl/sysdep.h
> File sysdeps/nacl/sysdep.h (right):
> 
>
https://codereview.chromium.org/759833008/diff/1/sysdeps/nacl/sysdep.h#newcod...
> sysdeps/nacl/sysdep.h:601: for (len = 0; buf[len] != '\0'; ++len);
> On 2014/12/08 20:56:48, Roland McGrath wrote:
> > Why is it safe to presume it's terminated and not check size?
> > Either way, this should just call strlen or __strnlen rather than using a
> loop.
> 
> Because the __nacl_irt_getcwd will fail if it couldn't fit the enture
> null terminiated string in buf.  So we could have returned already
> above in this case.

It still doesn't seem wise to presume such so as to crash if the lower layer did
wrong.
Since you have the upper bound on the string length, why not enforce it?

> I will try strlen.  I was assuming whoever wrote this has a good reason
> for using a loop.

If they had a good reason and wanted us to respect it, they would have written a
comment.

[Roland McGrath, 2014-12-08 21:17:43]

Conversely, of course, if the "+ 1" is all it takes to fix this bug, you could
leave the rest as is rather than cleaning it up.

[Sam Clegg, 2014-12-08 22:38:40]

https://codereview.chromium.org/759833008/diff/1/sysdeps/nacl/sysdep.h
File sysdeps/nacl/sysdep.h (right):

https://codereview.chromium.org/759833008/diff/1/sysdeps/nacl/sysdep.h#newcod...
sysdeps/nacl/sysdep.h:601: for (len = 0; buf[len] != '\0'; ++len);
On 2014/12/08 21:10:29, Sam Clegg wrote:
> On 2014/12/08 20:56:48, Roland McGrath wrote:
> > Why is it safe to presume it's terminated and not check size?
> > Either way, this should just call strlen or __strnlen rather than using a
> loop.
> 
> Because the __nacl_irt_getcwd will fail if it couldn't fit the enture
> null terminiated string in buf.  So we could have returned already
> above in this case.
> 
> I will try strlen.  I was assuming whoever wrote this has a good reason
> for using a loop.

Ok. Done.  Although in the case when __nacl_irt_getcwd violates its contract and
doesn't NULL terminate there isn't much we can do here.. somebody further up the
callstack is going to have hard time.

[Roland McGrath, 2014-12-08 22:57:06]

lgtm with -1 adjustment

https://codereview.chromium.org/759833008/diff/20001/sysdeps/nacl/sysdep.h
File sysdeps/nacl/sysdep.h (right):

https://codereview.chromium.org/759833008/diff/20001/sysdeps/nacl/sysdep.h#ne...
sysdeps/nacl/sysdep.h:602: return __strnlen(buf, size) + 1;
I think this should be size - 1: strnlen returns MAX(strlen, size), but here
size should include space for the terminator.  IOW, in the bad case you'd return
size+1, which cannot be right.

[Sam Clegg, 2014-12-08 23:29:54]

On 2014/12/08 22:57:06, Roland McGrath wrote:
> lgtm with -1 adjustment
> 
> https://codereview.chromium.org/759833008/diff/20001/sysdeps/nacl/sysdep.h
> File sysdeps/nacl/sysdep.h (right):
> 
>
https://codereview.chromium.org/759833008/diff/20001/sysdeps/nacl/sysdep.h#ne...
> sysdeps/nacl/sysdep.h:602: return __strnlen(buf, size) + 1;
> I think this should be size - 1: strnlen returns MAX(strlen, size), but here
> size should include space for the terminator.  IOW, in the bad case you'd
return
> size+1, which cannot be right.

I will to this, but I don't really see how this helps anyone in practice.

In the case when the '\0' is not found, __nacl_irt_getcwd has violated its
contract by either not terminated the string or written off the end of the
buffer.  In either case we are screwed and the damage has been done hasn't it?

For us to return anything less then strlen(buf) + 1 seems like just making
things worse and pretending things are OK when they are not.

Seems to be that it would make for clearer code to assume that __nacl_irt_getcwd
works as advertise and just return strlen(buf) + 1.

[Sam Clegg, 2014-12-09 02:07:58]

Committed patchset #3 (id:40001) manually as
1106743ae743989ed845d2bf5156535fbf2c7383 (presubmit successful).