> Sent: Thursday, January 02, 2020 at 4:26 PM
> From: "Otto Moerbeek" <o...@drijf.net>
> To: "Aham Brahmasmi" <aham.brahma...@gmx.com>
> Cc: misc@openbsd.org
> Subject: 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" <o...@drijf.net>
> > > To: "Aham Brahmasmi" <aham.brahma...@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
Ah ok, CAVEATS. My bad. The return value of the function should be
checked for possible error conditions. Dank je Otto.
So, if I understand correctly, checking for truncation implies ensuring
that the input string was not truncated to fit in the buffer.
So, the modified line would be:
int ret = snprintf(buf, sizeof(buf), "%s -S", path_rdistd);
if (ret < 0) {
// Some error has occurred.
// goto error;
}
if (ret >= sizeof(buf)) {
// The input string is longer than the size of the buffer and
// hence has been truncated to fit into the buffer.
// goto toolong;
}
Is my understanding of the check for truncation correct?
Dhanyavaad,
ab
---------|---------|---------|---------|---------|---------|---------|--
