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
_______________________________________________
networkmanager-list mailing list
[email protected]
http://mail.gnome.org/mailman/listinfo/networkmanager-list
