Re: [PATCH 1/4] conf: Copy up to MAXDNSRCH - 1 bytes, not MAXDNSRCH

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 27 Jun 2024 10:45:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Thu, Jun 27, 2024 at 01:45:33AM +0200, Stefano Brivio wrote:
> > Spotted by Coverity just recently. Not that it really matters as
> > MAXDNSRCH always appears to be defined as 1025, while a full domain
> > name can have up to 253 characters: it would be a bit pointless to
> > have a longer search domain.
> > 
> > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> 
> Hm.  So, IIRC strncpy() won't \0 terminate in the case where it
> truncates.  I guess we'll get away with that here since we expect
> c->dns_search to be filled with \0 before hand.  That's... more
> fragile than ideal, though.

Well, we know we start from a zero-initialised area, that's by design,
it's not that we get away with it. Without that consideration not many
things would work in this function.

Are you suggesting to use snprintf()? It looks a bit pedantic to me but
I'm fine with it. Otherwise, feel free to post a patch fixing it in a
way you feel it's ideal...

> > ---
> >  conf.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/conf.c b/conf.c
> > index e1f5422..9e47e9a 100644
> > --- a/conf.c
> > +++ b/conf.c
> > @@ -453,7 +453,7 @@ static void get_dns(struct ctx *c)
> >  			while (s - c->dns_search < ARRAY_SIZE(c->dns_search) - 1
> >  			       /* cppcheck-suppress strtokCalled */
> >  			       && (p = strtok(NULL, " \t"))) {
> > -				strncpy(s->n, p, sizeof(c->dns_search[0]));
> > +				strncpy(s->n, p, sizeof(c->dns_search[0]) - 1);
> >  				s++;
> >  				*s->n = 0;
> >  			}  
> 

-- 
Stefano