Re: [Dnsmasq-discuss] [PATCH] Fix some errors and warnings from clang-analyzer
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árwrote: > 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
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
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
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
--- 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