Fix issues found by gcc -fanalyze

Found via gcc -fanalyzer. Analyzer claims that it's possible certain functions may receive a NULL pointer when handling CLI arguments. Adding NULL pointer checks to correct the issues.

Signed-off-by Nicholas Vinson <[email protected]>

ethtool.c +5 -10

@@ -6182,16 +6182,18 @@ static int find_option(char *arg)
 	size_t len;
 	int k;
 
-	for (k = 1; args[k].opts; k++) {
-		opt = args[k].opts;
-		for (;;) {
-			len = strcspn(opt, "|");
-			if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
-				return k;
-
-			if (opt[len] == 0)
-				break;
-			opt += len + 1;
+	if (arg) {
+		for (k = 1; args[k].opts; k++) {
+			opt = args[k].opts;
+			for (;;) {
+				len = strcspn(opt, "|");

Quoted 5 lines · Nicholas Vinson

Found via gcc -fanalyzer. Analyzer claims that it's possible certain functions may receive a NULL pointer when handling CLI arguments. Adding NULL pointer checks to correct the issues.

Signed-off-by Nicholas Vinson <[email protected]>

A similar theoretical issue was discussed recently:

   https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

My position is still the same: argv[] members cannot be actually null unless there is a serious kernel bug (see the link above for an explanation). I'm not opposed to doing a sanity check just in case but if we do, I believe we should check the whole argv[] array right at the beginning and be done with it rather than add specific checks to random places inside parser code.

Quoted 19 lines

ethtool.c +5 -10

@@ -6182,16 +6182,18 @@ static int find_option(char *arg)
  	size_t len;
  	int k;
 
-	for (k = 1; args[k].opts; k++) {
-		opt = args[k].opts;
-		for (;;) {
-			len = strcspn(opt, "|");
-			if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
-				return k;
-
-			if (opt[len] == 0)
-				break;
-			opt += len + 1;
+	if (arg) {
+		for (k = 1; args[k].opts; k++) {
+			opt = args[k].opts;
+			for (;;) {
+				len = strcspn(opt, "|");

ethtool.c static int find_option(char *arg) · line 6199

Quoted 10 lines

ethtool.c +7 -0

+				if (strncmp(arg, opt, len) == 0 && arg[len] == 0)
+					return k;
+
+				if (opt[len] == 0)
+					break;
+				opt += len + 1;
+			}
  		}
  	}
 

I would rather prefer a simple

	if (!arg)
		return -1;

to avoid closing almost all of the function body inside an if block.

Michal

Quoted 8 lines · Michal Kubecek

Quoted 5 lines · Nicholas Vinson

Found via gcc -fanalyzer. Analyzer claims that it's possible certain functions may receive a NULL pointer when handling CLI arguments. Adding NULL pointer checks to correct the issues.

Signed-off-by Nicholas Vinson <[email protected]>

A similar theoretical issue was discussed recently:

Quoted 8 lines

   https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

My position is still the same: argv[] members cannot be actually null unless there is a serious kernel bug (see the link above for an explanation). I'm not opposed to doing a sanity check just in case but if we do, I believe we should check the whole argv[] array right at the beginning and be done with it rather than add specific checks to random places inside parser code.

By convention and POSIX standard, the last argv[] member is always set to NULL and is accessed from main(int argc, char **argp) via argp[argc] (see
https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html).
It's also possible for argc to be zero. In such a case, find_option(NULL) would get called.

However, after reviewing main(), I recommend changing:

        if (argc == 0)

                exit_bad_args();

to

        if (argc <= 0 || !*argp)

                exit_bad_args();

as this fixes the potential issue of main()'s argc being 0 (argc would be -1 at this point in such cases), and "!*argp" silences gcc's built-in analyzer (and should silence all other SA with respect to the reported issue) as the SA doesn't recognize that it would take a buggy execve implementation to allow argp to be NULL at this point ).

If you don't have any objections to this change, I can draft an updated patch to make this change.

Quoted 17 lines · Nicholas Vinson

Quoted 8 lines · Michal Kubecek

Quoted 5 lines · Nicholas Vinson

Found via gcc -fanalyzer. Analyzer claims that it's possible certain functions may receive a NULL pointer when handling CLI arguments. Adding NULL pointer checks to correct the issues.

Signed-off-by Nicholas Vinson <[email protected]>

A similar theoretical issue was discussed recently:

Quoted 8 lines

    https://patchwork.kernel.org/project/netdevbpf/patch/[email protected]/

My position is still the same: argv[] members cannot be actually null unless there is a serious kernel bug (see the link above for an explanation). I'm not opposed to doing a sanity check just in case but if we do, I believe we should check the whole argv[] array right at the beginning and be done with it rather than add specific checks to random places inside parser code.

Quoted 5 lines

By convention and POSIX standard, the last argv[] member is always set to NULL and is accessed from main(int argc, char **argp) via argp[argc] (see
https://pubs.opengroup.org/onlinepubs/9699919799/functions/execve.html).
It's also possible for argc to be zero. In such a case, find_option(NULL) would get called.

Please note that ethtool is not a utility for a general POSIX system. It is a very specific utility which works and makes sense only on Linux. That's why it can and does take many assumptions which are only guaranteed on Linux but may not be true on other POSIX systems.

Quoted 19 lines

However, after reviewing main(), I recommend changing:

        if (argc == 0)

                exit_bad_args();

to

        if (argc <= 0 || !*argp)

                exit_bad_args();

as this fixes the potential issue of main()'s argc being 0 (argc would be -1 at this point in such cases), and "!*argp" silences gcc's built-in analyzer (and should silence all other SA with respect to the reported issue) as the SA doesn't recognize that it would take a buggy execve implementation to allow argp to be NULL at this point ).

Quoted 2 lines

If you don't have any objections to this change, I can draft an updated patch to make this change.

As I said before, I do not object to adding a sanity check of argc/argv, I only objected to adding random checks inside the parser code just to make static checkers happy. If you want to add (or strengthen) a sanity check right at the beginning of the program, before any parsing, I have no problem with that.

Michal