[Dnsmasq-discuss] [PATCH] Remove NULL check for intname.

[Rosen Penev]

The function assumes intname is not NULL. Declaring it with __attribute__ ((nonnull)) exposes the issue.
---
src/network.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/network.c b/src/network.c
index f6ac03b..b9786ad 100644
--- a/src/network.c
+++ b/src/network.c
@@ -1186,7 +1186,6 @@ int random_sock(int family)
return -1;
}

-
int local_bind(int fd, union mysockaddr *addr, char *intname, int is_tcp)
{
union mysockaddr addr_copy = *addr;
@@ -1239,7 +1238,7 @@ static struct serverfd *allocate_sfd(union mysockaddr *addr, char *intname)
#endif
}

- if (intname && strlen(intname) != 0)
+ if (!strlen(intname))
ifindex = if_nametoindex(intname); /* index == 0 when not binding to an interface */

/* may have a suitable one already */

[Kurt H Maier]

Post by Rosen Penev
- if (intname && strlen(intname) != 0)
+ if (!strlen(intname))
ifindex = if_nametoindex(intname); /* index == 0 when not binding to an interface */

How much testing have you done of these patches?

khm

[r***@gmail.com]

Post by Kurt H Maier

Post by Rosen Penev
- if (intname && strlen(intname) != 0)
+ if (!strlen(intname))
ifindex = if_nametoindex(intname); /* index == 0 when not
binding to an interface */

How much testing have you done of these patches?

Compile time. Looks fine to me. To add to my commit message,
strlen(NULL) causes a segmentation fault, meaning intname cannot be
NULL.

Actually I wonder how many more warnings I can find like this...

Post by Kurt H Maier
khm
_______________________________________________
Dnsmasq-discuss mailing list
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Kevin Darbyshire-Bryant]

Post by r***@gmail.com

Post by Kurt H Maier

Post by Rosen Penev
- if (intname && strlen(intname) != 0)
+ if (!strlen(intname))
ifindex = if_nametoindex(intname); /* index == 0 when not
binding to an interface */

How much testing have you done of these patches?

Compile time. Looks fine to me. To add to my commit message,
strlen(NULL) causes a segmentation fault, meaning intname cannot be
NULL.

Which is precisely why the 'intname' guard is there. C evaluation order
is left to right, if 'intname' is NULL then evaluation stops there....
strlen is never called with a NULL intname.

Kevin

[Kurt H Maier]

Post by r***@gmail.com
Compile time. Looks fine to me. To add to my commit message,
strlen(NULL) causes a segmentation fault, meaning intname cannot be
NULL.
Actually I wonder how many more warnings I can find like this...

Probably a lot, but you should stop.

Forgive me, but this is code you don't appear to understand and have not
even tested. Some of it introduces nasty side effects and some of it
even outright inverts logic. Blindly pasting static analysis output is
not how programs are maintained, but it does introduce a lot of work for
those of us who care sufficiently to review the code.

This code introduces bugs and does not even have the grace to improve
functionality in the process.

khm

[Roy Marples]

Post by Rosen Penev
@@ -1239,7 +1238,7 @@ static struct serverfd *allocate_sfd(union mysockaddr *addr, char *intname)
#endif
}
- if (intname && strlen(intname) != 0)
+ if (!strlen(intname))
ifindex = if_nametoindex(intname); /* index == 0 when not binding to an interface */
/* may have a suitable one already */

I have no comment on the functionality of the patch (it if intname needs
to be NULL checked or not), but this is not a good use of strlen.

This could be re-written as

if (*intname != '\0')

Which saves a function call because the actual length of the string is
not used.

Roy

[Kevin Lyda]

That's not actually correct in practice. If you'd like to see that I'm
correct take the following two programs:

foo.c:
#include <string.h>
#include <stdio.h>

int main(int argc, char *argv[]) {
if (strlen(argv[0]) == 0) {
printf("Command empty");
} else {
printf("Command not empty");
}
}
bar.c:
#include <string.h>
#include <stdio.h>

int main(int argc, char *argv[]) {
if (*argv[0] == '\0') {
printf("Command empty");
} else {
printf("Command not empty");
}
}

Next compile them to assembly like so:

for f in {foo,bar}.c; do gcc -O2 -S $f; done

And then compare them:

diff {foo,bar}.s

They should be the same (possibly different by a ".file" line). And if you
inspect the resulting code, there won't be a call to strlen or to any
function at all (well, except printf).

The reason is that gcc, like most C compilers since the 90s, optimises a
number of common functions in the standard C library. Which means that
developers can stick with writing code that will be well-optimised *and*
highly readable.

Kevin

Post by Rosen Penev

Post by Rosen Penev
@@ -1239,7 +1238,7 @@ static struct serverfd *allocate_sfd(union

mysockaddr *addr, char *intname)

Post by Rosen Penev
#endif
}
- if (intname && strlen(intname) != 0)
+ if (!strlen(intname))
ifindex = if_nametoindex(intname); /* index == 0 when not binding

to an interface */

Post by Rosen Penev
/* may have a suitable one already */

I have no comment on the functionality of the patch (it if intname needs
to be NULL checked or not), but this is not a good use of strlen.
This could be re-written as
if (*intname != '\0')
Which saves a function call because the actual length of the string is
not used.
Roy
_______________________________________________
Dnsmasq-discuss mailing list
http://lists.thekelleys.org.uk/mailman/listinfo/dnsmasq-discuss

[Roy Marples]

Post by Kevin Lyda
That's not actually correct in practice.

And top posting is?

Post by Kevin Lyda
If you'd like to see that I'm
#include <string.h>
#include <stdio.h>
int main(int argc, char *argv[]) {
if (strlen(argv[0]) == 0) {
printf("Command empty");
} else {
printf("Command not empty");
}
}
#include <string.h>
#include <stdio.h>
int main(int argc, char *argv[]) {
if (*argv[0] == '\0') {
printf("Command empty");
} else {
printf("Command not empty");
}
}
for f in {foo,bar}.c; do gcc -O2 -S $f; done

Interesting that you assume I use bash and gcc.
This is more portable.
for f in foo bar; do cc -S $f.c; done

Post by Kevin Lyda
diff {foo,bar}.s
They should be the same (possibly different by a ".file" line). And if
you inspect the resulting code, there won't be a call to strlen or to
any function at all (well, except printf).

netbsd$ diff foo.s bar.s
2c2
< .file "foo.c"
---

Post by Kevin Lyda
.file "bar.c"

22,24c22,24
< movq (%rsi), %rdi
< callq strlen
< cmpq $0, %rax
---

Post by Kevin Lyda
movq (%rsi), %rsi
movsbl (%rsi), %edi
cmpl $0, %edi

Now, if I do apply -O2 then clang (my system compiler) does indeed
optimise it away.
What is worring is that even with -O0 or no -O gcc *always* optimises it
away. What if I want to call the function to be called regardless?

Post by Kevin Lyda
The reason is that gcc, like most C compilers since the 90s, optimises a
number of common functions in the standard C library. Which means that
developers can stick with writing code that will be well-optimised *and*
highly readable.

Spoken like a man who has never dealt with compiler issues!

if (strlen(foo) != 0)
if the function to calculate the length of the string is not zero

if (*foo != '\0')
if the first character of the string is not NUL

I'm having a hard time beliving that the former is more readable AND
just as performant.
It has more text for the brain to digest and a man page to read per
platform.

Or are you going to assume that gcc is the only compiler ever used anywhere?

Roy

[Kevin Lyda]

Post by Roy Marples

Post by Kevin Lyda
That's not actually correct in practice.

And top posting is?

Yes. It is. We lost that battle, sorry. If it worries you, t-prot is a
thing.

Interesting that you assume I use bash and gcc.

Post by Roy Marples
This is more portable.
for f in foo bar; do cc -S $f.c; done

I use zsh actually, and you figured out what I meant. Neither version will
work in csh style shells. And an odd complaint about a shortcut here, since
later you claim more text makes it harder to read. Also I specified gcc
specifically because I had only tested with gcc - though from past
experience I know modern compilers like clang, icc and others also do such
optimisation.

Also you removed the -O2 that I had in there.

Post by Roy Marples
Now, if I do apply -O2 then clang (my system compiler) does indeed
optimise it away.
What is worring is that even with -O0 or no -O gcc *always* optimises it
away. What if I want to call the function to be called regardless?

Ugh, and my tests with gcc -O0 were actually with clang because Apple links
gcc to clang. Checking gcc -O0 on Linux shows that it does optimize even at
-O2.

Right, so to get foo.c to actually call strlen with gcc:

gcc -fno-builtin-strlen -S foo.c

or more generically:

gcc -fno-builtin -S foo.c

Post by Roy Marples
Spoken like a man who has never dealt with compiler issues!

A rather funny comment since I'm currently dealing with multiple compiler
issues.

if (strlen(foo) != 0)

Post by Roy Marples
if the function to calculate the length of the string is not zero
if (*foo != '\0')
if the first character of the string is not NUL
I'm having a hard time beliving that the former is more readable AND
just as performant.
It has more text for the brain to digest and a man page to read per
platform.

Where "more text" is four characters and replaces symbols for words that
are easy for the eye to skim. Maybe I've lead a sheltered career for the
past three decades, but I don't really recall a lot of APL shops in my
travels.

Or are you going to assume that gcc is the only compiler ever used anywhere?
Nope. Never do. Though I do like it a great deal more than VMS's C
compiler. I do wish Apple wouldn't point gcc at clang though.

Regardless, the dnsmasq Makefile sets -O2 in its CFLAGS so gcc or clang or,
as I said previously, any modern C compiler, it will optimise out the
strlen call and the strlen call is easier to read.

Kevin