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

[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];

[Pali Rohár]

Post by Rosen Penev
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.

[r***@gmail.com]

Post by Pali Rohár

Post by Rosen Penev
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?

Post by Pali Rohár
Also strncpy in some circumstances fill string which is not null
terminated.

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

[Pali Rohár]

Post by r***@gmail.com

Post by Pali Rohár

Post by Rosen Penev
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.

Post by r***@gmail.com

Post by Pali Rohár
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.

[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

Post by Pali Rohár

Post by r***@gmail.com

Post by Pali Rohár

Post by Rosen Penev
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.

Post by r***@gmail.com

Post by Pali Rohár
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