Re: [Dnsmasq-discuss] [PATCH] Fix some errors and warnings from clang-analyzer

[Rosen Penev]

Zombie revive:

I was checking a different project with clang-analyzer which reminded me of
this.

Line in question that it complained about was this one:

strncat (build_opts_new, " -cl-opt-disable", sizeof (build_opts_new) - 1);

The error was:

Potential buffer overflow. Replace with 'sizeof(build_opts_new) -
strlen(build_opts_new) - 1' or use a safer 'strlcat' API

On Thu, Oct 5, 2017 at 1:46 AM Pali Rohár  wrote:

> On Thursday 05 October 2017 01:41:02 ros...@gmail.com wrote:
> > On Thu, 2017-10-05 at 09:48 +0200, Pali Rohár wrote:
> > > On Wednesday 04 October 2017 19:22:11 Rosen Penev wrote:
> > > > diff --git a/src/cache.c b/src/cache.c
> > > > index 4f43246..88851e7 100644
> > > > --- a/src/cache.c
> > > > +++ b/src/cache.c
> > > > @@ -572,7 +572,7 @@ struct crec *cache_insert(char *name, struct
> > > > all_addr *addr,
> > > >  }
> > > >
> > > >if (name)
> > > > -strcpy(cache_get_name(new), name);
> > > > +strncpy(cache_get_name(new), name,
> > > > strlen(cache_get_name(new)));
> > >
> > > Hi! This line looks suspicious. Should not be length argument sizeof
> > > of
> > > destination buffer, instead of current length of null term string?
> > >
> >
> > Doesn't sizeof on a pointer return the size of the pointer instead of
> > the string?
>
> Yes! Therefore I wrote word construction "size of destination buffer"
> and not code "sizeof(buf)". You need to take size of that destination
> buffer manually.
>
> > > Also strncpy in some circumstances fill string which is not null
> > > terminated.
> >
> > strlen apparently does not include \0. needs a + 1 looks like.
>
> That is another problem. But still my above sentence about strncpy
> applies. strncpy does not ensure that destination string would be always
> null terminated, even source one was.
>
> --
> Pali Rohár
> pali.ro...@gmail.com
>
___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [PATCH] Fix some errors and warnings from clang-analyzer

[rosenp]

On Thu, 2017-10-05 at 09:48 +0200, Pali Rohár wrote:
> On Wednesday 04 October 2017 19:22:11 Rosen Penev wrote:
> > diff --git a/src/cache.c b/src/cache.c
> > index 4f43246..88851e7 100644
> > --- a/src/cache.c
> > +++ b/src/cache.c
> > @@ -572,7 +572,7 @@ struct crec *cache_insert(char *name, struct
> > all_addr *addr,
> >  }
> >  
> >if (name)
> > -strcpy(cache_get_name(new), name);
> > +strncpy(cache_get_name(new), name,
> > strlen(cache_get_name(new)));
> 
> Hi! This line looks suspicious. Should not be length argument sizeof
> of
> destination buffer, instead of current length of null term string?
> 

Doesn't sizeof on a pointer return the size of the pointer instead of
the string?
> Also strncpy in some circumstances fill string which is not null
> terminated.

strlen apparently does not include \0. needs a + 1 looks like.
> 

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [PATCH] Fix some errors and warnings from clang-analyzer

[Pali Rohár]

On Wednesday 04 October 2017 19:22:11 Rosen Penev wrote:
> diff --git a/src/cache.c b/src/cache.c
> index 4f43246..88851e7 100644
> --- a/src/cache.c
> +++ b/src/cache.c
> @@ -572,7 +572,7 @@ struct crec *cache_insert(char *name, struct all_addr 
> *addr,
>  }
>  
>if (name)
> -strcpy(cache_get_name(new), name);
> +strncpy(cache_get_name(new), name, strlen(cache_get_name(new)));

Hi! This line looks suspicious. Should not be length argument sizeof of
destination buffer, instead of current length of null term string?

Also strncpy in some circumstances fill string which is not null
terminated.

-- 
Pali Rohár
pali.ro...@gmail.com

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

Re: [Dnsmasq-discuss] [PATCH] Fix some errors and warnings from clang-analyzer

[Pali Rohár]

On Thursday 05 October 2017 01:41:02 ros...@gmail.com wrote:
> On Thu, 2017-10-05 at 09:48 +0200, Pali Rohár wrote:
> > On Wednesday 04 October 2017 19:22:11 Rosen Penev wrote:
> > > diff --git a/src/cache.c b/src/cache.c
> > > index 4f43246..88851e7 100644
> > > --- a/src/cache.c
> > > +++ b/src/cache.c
> > > @@ -572,7 +572,7 @@ struct crec *cache_insert(char *name, struct
> > > all_addr *addr,
> > >  }
> > >  
> > >if (name)
> > > -strcpy(cache_get_name(new), name);
> > > +strncpy(cache_get_name(new), name,
> > > strlen(cache_get_name(new)));
> > 
> > Hi! This line looks suspicious. Should not be length argument sizeof
> > of
> > destination buffer, instead of current length of null term string?
> > 
> 
> Doesn't sizeof on a pointer return the size of the pointer instead of
> the string?

Yes! Therefore I wrote word construction "size of destination buffer"
and not code "sizeof(buf)". You need to take size of that destination
buffer manually.

> > Also strncpy in some circumstances fill string which is not null
> > terminated.
> 
> strlen apparently does not include \0. needs a + 1 looks like.

That is another problem. But still my above sentence about strncpy
applies. strncpy does not ensure that destination string would be always
null terminated, even source one was.

-- 
Pali Rohár
pali.ro...@gmail.com

___
Dnsmasq-discuss mailing list
Dnsmasq-discuss@lists.thekelleys.org.uk
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss