Replace readlink calls with a safer version that guarantees NULL-termination.

Replace readlink calls with a safer version that guarantees NULL-termination.

This patch is part of a bigger patch that helps merging the breakpad code
with the modified version in Chromium OS.

Specifically, this patch makes the following changes:
1. Add a SafeReadLink function that wraps sys_readlink() to resolve a
   symbolic link but guarantees the result is NULL-terminated on success.
2. Refactor other source code to use SafeReadLink instead of readlink()
   or sys_readlink().

BUG=455
TEST=Tested the following:
1. Build on 32-bit and 64-bit Linux with gcc 4.4.3 and gcc 4.6.
2. Build on Mac OS X 10.6.8 with gcc 4.2 and clang 3.0 (with latest gmock).
3. All unit tests pass.
4. Run minidump-2-core to covnert a minidump file to a core file.

[Ben Chan]

http://breakpad.appspot.com/334001/diff/1/4
File src/client/linux/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/334001/diff/1/4#newcode83
Line 83: char buf[256];
Should it be changed to NAME_MAX / PATH_MAX instead of hardcoding to 256?

http://breakpad.appspot.com/334001/diff/1/5
File src/client/linux/minidump_writer/linux_dumper.cc (right):

http://breakpad.appspot.com/334001/diff/1/5#newcode537
Line 537: char exe_link[NAME_MAX];
We should probably consider using PATH_MAX instead of NAME_MAX unless the stack
size is a concern.

[Lei Zhang (chromium)]

http://breakpad.appspot.com/334001/diff/1/4
File src/client/linux/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/334001/diff/1/4#newcode83
Line 83: char buf[256];
On 2011/12/19 21:17:24, Ben Chan wrote:
> Should it be changed to NAME_MAX / PATH_MAX instead of hardcoding to 256?

SGTM

http://breakpad.appspot.com/334001/diff/1/5
File src/client/linux/minidump_writer/linux_dumper.cc (right):

http://breakpad.appspot.com/334001/diff/1/5#newcode537
Line 537: char exe_link[NAME_MAX];
On 2011/12/19 21:17:24, Ben Chan wrote:
> We should probably consider using PATH_MAX instead of NAME_MAX unless the
stack
> size is a concern.

Yes please.

http://breakpad.appspot.com/334001/diff/1/10
File src/common/linux/safe_readlink.h (right):

http://breakpad.appspot.com/334001/diff/1/10#newcode45
Line 45: // sys_readlink(). |buffer_size| is used as an input and output
argument,
Does |buffer_size| need to be an out argument? We don't use the out value
anywhere except in tests.

http://breakpad.appspot.com/334001/diff/1/10#newcode47
Line 47: // the number of bytes modified modified in |buffer| (i.e. the length
of
nit: s/modified modified/modified/

[Ben Chan]

http://breakpad.appspot.com/334001/diff/1/4
File src/client/linux/crash_generation/crash_generation_server.cc (right):

http://breakpad.appspot.com/334001/diff/1/4#newcode83
Line 83: char buf[256];
On 2011/12/19 21:58:31, thestig wrote:
> On 2011/12/19 21:17:24, Ben Chan wrote:
> > Should it be changed to NAME_MAX / PATH_MAX instead of hardcoding to 256?
> 
> SGTM

Done.

http://breakpad.appspot.com/334001/diff/1/5
File src/client/linux/minidump_writer/linux_dumper.cc (right):

http://breakpad.appspot.com/334001/diff/1/5#newcode537
Line 537: char exe_link[NAME_MAX];
On 2011/12/19 21:58:31, thestig wrote:
> On 2011/12/19 21:17:24, Ben Chan wrote:
> > We should probably consider using PATH_MAX instead of NAME_MAX unless the
> stack
> > size is a concern.
> 
> Yes please.

Haven't changed this one yet as it affects all code related to MappingInfo.

http://breakpad.appspot.com/334001/diff/1/10
File src/common/linux/safe_readlink.h (right):

http://breakpad.appspot.com/334001/diff/1/10#newcode45
Line 45: // sys_readlink(). |buffer_size| is used as an input and output
argument,
On 2011/12/19 21:58:31, thestig wrote:
> Does |buffer_size| need to be an out argument? We don't use the out value
> anywhere except in tests.

I considered a few approaches but haven't decided which one is better, so wanna
discuss here. Comments/suggestions are welcome.

(1) Follow readlink() exactly and return a ssize_t value, so the caller can
check if the returned value >= for success. But I somehow feel the
ssize_t/size_t comparison is a bit messy.

(2) The current form where SafeReadLink returns a Boolean to indicate
success/failure (as we always want to check for error, but may not care about
the result length), and have |buffer_size| as an in/out argument to return the
result length so that we don't need to call strlen(buffer) again to determine
that.

(3) Simply drop the |buffer_size| argument and require  |buffer| always has a
length of PATH_MAX. Unless we are certain about the length of the resolved path
or need to conserve stack space (as PATH_MAX could be 4096 and that may be
prohibitive for embedded systems unless PATH_MAX is defined to be a smaller
value on those systems), it seems reasonable to have that requirement.


Also, I just realized that PATH_MAX is defined to include the NULL byte, so I
don't need to use PATH_MAX+1 as the buffer size.

http://breakpad.appspot.com/334001/diff/1/10#newcode47
Line 47: // the number of bytes modified modified in |buffer| (i.e. the length
of
On 2011/12/19 21:58:31, thestig wrote:
> nit: s/modified modified/modified/

Done.

[Lei Zhang (chromium)]

On 2011/12/19 22:54:22, Ben Chan wrote:
> On 2011/12/19 21:58:31, thestig wrote:
> > Does |buffer_size| need to be an out argument? We don't use the out value
> > anywhere except in tests.
> 
> I considered a few approaches but haven't decided which one is better, so
wanna
> discuss here. Comments/suggestions are welcome.
> 
> (1) Follow readlink() exactly and return a ssize_t value, so the caller can
> check if the returned value >= for success. But I somehow feel the
> ssize_t/size_t comparison is a bit messy.
> 
> (2) The current form where SafeReadLink returns a Boolean to indicate
> success/failure (as we always want to check for error, but may not care about
> the result length), and have |buffer_size| as an in/out argument to return the
> result length so that we don't need to call strlen(buffer) again to determine
> that.
> 
> (3) Simply drop the |buffer_size| argument and require  |buffer| always has a
> length of PATH_MAX. Unless we are certain about the length of the resolved
path
> or need to conserve stack space (as PATH_MAX could be 4096 and that may be
> prohibitive for embedded systems unless PATH_MAX is defined to be a smaller
> value on those systems), it seems reasonable to have that requirement.

Given our choices, (2) looks the best to me, so LGTM to patch set 2.

> Also, I just realized that PATH_MAX is defined to include the NULL byte, so I
> don't need to use PATH_MAX+1 as the buffer size.

Good catch.

[Ted Mielczarek]

Just one comment, LGO

http://breakpad.appspot.com/334001/diff/3003/4008
File src/common/linux/safe_readlink.cc (right):

http://breakpad.appspot.com/334001/diff/3003/4008#newcode39
Line 39: bool SafeReadLink(const char* path, char* buffer, size_t* buffer_size)
{
I agree that buffer_size should just be an input param. I think you could go one
step further and add a helper template in the header that avoids the need to
pass the size when passing an array, like:
template<size_t N>
bool SafeReadLink(const char* path, char (&buffer)[N]) {
  return SafeReadLink(path, buffer, N);
}

Since most of the callers are passing arrays anyway, this would save one
parameter but still be safe.

[Ben Chan]

http://breakpad.appspot.com/334001/diff/3003/4008
File src/common/linux/safe_readlink.cc (right):

http://breakpad.appspot.com/334001/diff/3003/4008#newcode39
Line 39: bool SafeReadLink(const char* path, char* buffer, size_t* buffer_size)
{
On 2011/12/20 12:44:05, Ted Mielczarek wrote:
> I agree that buffer_size should just be an input param. I think you could go
one
> step further and add a helper template in the header that avoids the need to
> pass the size when passing an array, like:
> template<size_t N>
> bool SafeReadLink(const char* path, char (&buffer)[N]) {
>   return SafeReadLink(path, buffer, N);
> }
> 
> Since most of the callers are passing arrays anyway, this would save one
> parameter but still be safe.

Done.

Thanks for the inputs. I've changed the function based on the consensus such
that |buffer_size| is a simple input argument. Also added the template helper as
you suggested.