On Thu, Jan 02, 2020 at 07:45:25PM +0100, Aham Brahmasmi wrote:
> > 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
Look, I've done already enough hand-holding. There are pelnty pf
resources to educate yourself. At some point you need to stop asking
question and study.
-Otto
