Re: [PATCH] Fix unterminated buffer after readlink() call

[Dan Williams <dcbw redhat com>]

On Fri, 2011-10-14 at 17:26 +0200, Thomas Jarosch wrote:
> Hi Dan,
> 
> On 10/14/2011 05:14 PM, Dan Williams wrote:
> >> --- a/src/nm-device-ethernet.c
> >> +++ b/src/nm-device-ethernet.c
> >> @@ -343,6 +343,7 @@ _update_s390_subchannels (NMDeviceEthernet *self)
> >>  	while ((item = g_dir_read_name (dir))) {
> >>  		char buf[50];
> >>  		char *cdev_path;
> >> +		ssize_t link_len;
> >>  
> >>  		if (strncmp (item, "cdev", 4))
> >>  			continue;  /* Not a subchannel link */
> >> @@ -351,7 +352,9 @@ _update_s390_subchannels (NMDeviceEthernet *self)
> >>  
> >>  		memset (buf, 0, sizeof (buf));
> > 
> > My initial read is that the memset would terminate the buffer, since
> > we're passing sizeof (buf) - 1 into the call, the last byte of the
> > buffer will always be 0, no?  
> 
> Good catch, I missed the memset() call.
> 
> And if readlink() doesn't fill the buffer,
> > the remaining bytes will already be set to 0 by the memset, AFAICT.  Are
> > you seeing a crash here or was this from visual inspection?
> 
> Pure visual inspection, I was looking for readlink() usage patterns.
> 
> Still the buf[link_len] = '0'; is more explicit and we wouldn't need
> the memset() call at all. What do you think?

I try to do the memset() calls specifically to prevent bugs caused by
forgetting to do NULL termination later on; it's kind of the shotgun
approach but these paths generally aren't performance sensitive and I
think the tradeoff of a few cycles to memset (which is a very optimized
operation these days) is worth the decreased possibility of bugs and
buffer overruns here.  At least that was my thought :)

Dan