On Sat, 17 Oct 2009 07:46:21 +0900
Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
>
> On Oct 16, 2009, at 11:05 PM, Jeff Layton wrote:
>
> > On Tue, 13 Oct 2009 10:55:06 -0400
> > Chuck Lever <chuck.lever@xxxxxxxxxx> wrote:
> >> +
> >> +/*
> >> + * Stuff a 'struct mon' with callback data, and call @func.
> >> + *
> >> + * Returns the count of in-core records created.
> >> + */
> >> +static unsigned int
> >> +nsm_parse_line(const char *hostname, const time_t timestamp, char
> >> *line,
> >> + nsm_populate_t func)
> >> +{
> >> + struct sockaddr_in sin = {
> >> + .sin_family = AF_INET,
> >> + };
> >> + struct mon mon;
> >> + int count, i;
> >> + char *c;
> >> +
> >> + c = strchr(line, '\n');
> >> + if (c)
> >> + *c = '\0';
> >> +
> >> + count = sscanf(line, "%8x %8x %8x %8x ",
> >> + &sin.sin_addr.s_addr,
> >> + &mon.mon_id.my_id.my_prog,
> >> + &mon.mon_id.my_id.my_vers,
> >> + &mon.mon_id.my_id.my_proc);
> >> + if (count != 4)
> >> + return 0;
> >> +
> >> + c = line + 36;
> >> + for (i = 0; i < SM_PRIV_SIZE; i++) {
> >> + unsigned int tmp;
> >> + if (sscanf(c, "%2x", &tmp) != 1)
> >> + return 0;
> >> + mon.priv[i] = tmp;
> >> + c += 2;
> >> + }
> >> +
> >
> > ^^^
> > I think we need some bounds checking here. There doesn't seem to be
> > any
> > guarantee that the line we read in from the "db" is a particular
> > length. A check to verify that before trying to convert the cookie
> > would be good.
>
> You mean the line read from the file is possibly not long enough? I
> think sscanf(3) will bail and we'll hit the "return 0;" if it can't
> find enough bytes in the cookie. It would be worth adding a specific
> test or two for short db lines in the nascent statd regression/unit
> test suite.
>
Ok. Now that I look again, I think you're correct. In the event that
the line ends on the single space after the 4th field, c will point to
a NULL byte and sscanf will just bail out.
> However, any new checks we want in this particular spot should
> probably be added by subsequent patches (ie after the IPv6 support
> series is committed), so we can verify that the IPv6 changes don't
> alter the existing on-disk format logic -- they only copy it to a new
> location. No telling what kind of odd side effects other code outside
> of nfs-utils may depend on here.
>
Fair enough, that sounds reasonable. My main concern at this point was
simply to make sure that we didn't segfault if the DB file turned out
to be corrupt. I don't think that'll happen, AFAICT.
> >> + c++;
> >> + mon.mon_id.mon_name = c;
> >> + while (*c && *c != ' ')
> >> + c++;
> >> + if (*c)
> >> + *c++ = '\0';
> >> + while (*c == ' ')
> >> + c++;
> >> + mon.mon_id.my_id.my_name = c;
> >> +
> >> + return func(hostname, (struct sockaddr *)&sin, &mon, timestamp);
> >> +}
>
> --
> Chuck Lever
> chuck[dot]lever[at]oracle[dot]com
>
>
>
--
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
