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

2017-10-19 Thread 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

2017-10-05 Thread 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

2017-10-05 Thread 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

2017-10-05 Thread 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

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

2017-10-04 Thread Rosen Penev 
---
 src/cache.c   | 2 +-
 src/edns0.c   | 2 +-
 src/option.c  | 2 ++
 src/rfc1035.c | 5 -
 src/util.c| 2 +-
 5 files changed, 9 insertions(+), 4 deletions(-)

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)));
   else
 *cache_get_name(new) = 0;
 
diff --git a/src/edns0.c b/src/edns0.c
index 0bc45f9..74b94b6 100644
--- a/src/edns0.c
+++ b/src/edns0.c
@@ -268,7 +268,7 @@ static size_t add_dns_client(struct dns_header *header, 
size_t plen, unsigned ch
 {
   int maclen, replace = 2; /* can't get mac address, just delete any incoming. 
*/
   unsigned char mac[DHCP_CHADDR_MAX];
-  char encode[18]; /* handle 6 byte MACs */
+  char encode[18] = {0}; /* handle 6 byte MACs */
 
   if ((maclen = find_mac(l3, mac, 1, now)) == 6)
 {
diff --git a/src/option.c b/src/option.c
index be08a34..1feab22 100644
--- a/src/option.c
+++ b/src/option.c
@@ -4397,6 +4397,7 @@ static int one_file(char *file, int hard_opt)
 }
 
   read_file(file, f, hard_opt);
+  fclose(f);
   return 1;
 }
 
@@ -4515,6 +4516,7 @@ void read_servers_file(void)
   cleanup_servers();
 
   read_file(daemon->servers_file, f, LOPT_REV_SERV);
+  fclose(f);
 }
 
 
diff --git a/src/rfc1035.c b/src/rfc1035.c
index 7c17770..a0e971f 100644
--- a/src/rfc1035.c
+++ b/src/rfc1035.c
@@ -1184,7 +1184,10 @@ int add_resource_record(struct dns_header *header, char 
*limit, int *truncp, int
 
   case 'z':
sval = va_arg(ap, char *);
-   usval = sval ? strlen(sval) : 0;
+   if (sval == NULL)
+ break;
+   else
+ usval = sval ? strlen(sval) : 0;
if (usval > 255)
  usval = 255;
 CHECK_LIMIT(usval + 1);
diff --git a/src/util.c b/src/util.c
index 97cf1f4..5cff86f 100644
--- a/src/util.c
+++ b/src/util.c
@@ -518,7 +518,7 @@ int parse_hex(char *in, unsigned char *out, int maxlen,
  int j, bytes = (1 + (r - in))/2;
  for (j = 0; j < bytes; j++)
{
- char sav = sav;
+ char sav;
  if (j < bytes - 1)
{
  sav = in[(j+1)*2];
-- 
2.14.2


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