Re: Probable off by one in src/usr.bin/rdist/docmd.c

On Thu, Jan 02, 2020 at 03:39:53PM +0100, Aham Brahmasmi wrote:

> Hallo Otto,
>
> Dank je Otto for your helpful reply.
>
> > Sent: Wednesday, January 01, 2020 at 3:36 PM
> > From: "Otto Moerbeek" <otto@drijf.net>
> > To: "Aham Brahmasmi" <aham.brahmasmi@gmx.com>
> > Cc: misc@openbsd.org
> > Subject: Re: Probable off by one in src/usr.bin/rdist/docmd.c
> >
> > On Wed, Jan 01, 2020 at 04:02:24PM +0100, Aham Brahmasmi wrote:
> >
> > > Namaste misc,
> > >
> > > Question:
> > > In the makeconn function in src/usr.bin/rdist/docmd.c, should the 5 in
> > > the following line be replaced by 4?
> > > ...
> > > static int
> > > makeconn(char *rhost)
> > > {
> > > ...
> > > (void) snprintf(buf, sizeof(buf), "%.*s -S",
> > > (int)(sizeof(buf)-5), path_rdistd);
> > > ...
> > >
> > > Explanation:
> > > I have a limited ability to read code, so I may be wrong here.
> > >
> > > If I am not wrong, strings are terminated with '\0' which I think is a
> > > single byte. So, in the above case, the sizeof(" -S" + '\0')=4. But the
> > > code has 5.
> > >
> > > I am not sure of my "'\0' is a single byte" part, and hence my query.
> >
> > By definition, '\0' is a single byte. sizeof(String literal) included
> > the terminating '\0'. So sizeof("foo") is 4.
> >
> > The sizeof(buf)-5 fills in the precision of the %....s. That means
> > that path_rdistd wil be limited to that number of chars. The " -S"
> > part indeed takes 3 chars, so there is sizeof(buf) - 3 left for
> > path_rdistd, excluding the terminating '\0'. So -4 is indeed right.
>
> Understood.
>
> > Butt does it matter? I'd say no, only if path_rdistd is close to
> > BUFSIZ in length tunrcation will happen 1 char earlier than possible.
> > I would argue that specifying the precision here is rather confusing,
> > and it would be better to use the standard idiom equivalent to the
> > example in the snprintf man page.
>
> From the snprintf man page (https://man.openbsd.org/snprintf):
>
> ...
> int
> snprintf(char *str, size_t size, const char *format, ...);
> ...
>
> So, if I understand the standard idiom in the snprintf man page
> correctly, the modified line would be:
>
> (void) snprintf(buf, sizeof(buf), "%s -S", path_rdistd);
>
> Am I correct in my understanding?
>
> > -Otto
>
> Dhanyavaad,
> ab

No,

you want to check for truncation. See the CAVEATS section.

-Otto