[Bug 6716] Found code mistakes using cppcheck

[bugzilla at busybox.net]

https://bugs.busybox.net/show_bug.cgi?id=6716

--- Comment #3 from Denys Vlasenko <vda.linux at googlemail.com> 2013-11-29 15:54:11 UTC ---
You did find some bugs, and I'm applying some parts of the patch.

The parts I don't apply are:

--- a/applets/applet_tables.c
+++ b/applets/applet_tables.c
@@ -150,6 +150,7 @@ int main(int argc, char **argv)
                        if (!fp)
                                return 1;
                        fputs(line_new, fp);
+                       fclose(fp);
                }
        }

Yes, we intentionally don't close the file. Exiting will do it for us.


--- a/archival/gzip.c
+++ b/archival/gzip.c
@@ -2121,14 +2121,13 @@ int gzip_main(int argc, char **argv)
 int gzip_main(int argc UNUSED_PARAM, char **argv)
 #endif
 {
-       unsigned opt;
-
 #if ENABLE_FEATURE_GZIP_LONG_OPTIONS
        applet_long_options = gzip_longopts;
 #endif
-       /* Must match bbunzip's constants OPT_STDOUT, OPT_FORCE! */
-       opt = getopt32(argv, "cfv" IF_GUNZIP("dt") "q123456789n");
+
 #if ENABLE_GUNZIP /* gunzip_main may not be visible... */
+       /* Must match bbunzip's constants OPT_STDOUT, OPT_FORCE! */
+       unsigned opt = getopt32(argv, "cfv" IF_GUNZIP("dt") "q123456789n");
        if (opt & 0x18) // -d and/or -t
                return gunzip_main(argc, argv);
 #endif


getopt32() call has side-effects, we need them even if !ENABLE_GUNZIP.


--- a/shell/hush.c
+++ b/shell/hush.c
@@ -7196,11 +7196,15 @@ static NOINLINE int run_pipe(struct pipe *pi)
                        }
 #endif
                        if (pi->alive_cmds == 0 && pi->followup == PIPE_BG) {
+                               int fd;
                                /* 1st cmd in backgrounded pipe
                                 * should have its stdin /dev/null'ed */
                                close(0);
-                               if (open(bb_dev_null, O_RDONLY))
+                               if ( (fd = open(bb_dev_null, O_RDONLY)) >= 0 )
+                               {
+                                       close(fd);
                                        xopen("/", O_RDONLY);
+                               }
                        } else {
                                xmove_fd(next_infd, 0);
                        }

You misunderstood what code does.
"if (open(bb_dev_null, O_RDONLY))" is true ONLY if open() returned -1. It can't
return >0 here, because fd 0 is closed, and open() is required to return
*first* free fd on success.


--- a/networking/udhcp/d6_dhcpc.c
+++ b/networking/udhcp/d6_dhcpc.c
@@ -124,7 +124,7 @@ static void *d6_copy_option(uint8_t *option, uint8_t
*option_end, unsigned code)
 static void *d6_store_blob(void *dst, const void *src, unsigned len)
 {
        memcpy(dst, src, len);
-       return dst + len;
+       return &dst[ len ];
 }


This is not a change.


@@ -749,7 +749,8 @@ print_inetname(const struct sockaddr *from)
                        n = xmalloc_sockaddr2host_noport((struct
sockaddr*)from);
                }
                printf("  %s (%s)", (n ? n : ina), ina);
-               free(n);
+               if(n)
+                       free(n);
        }
        free(ina);
 }

free(NULL) is valid.



-    VALUE *l = l; /* silence gcc */
-    VALUE *v = v; /* silence gcc */
+    VALUE *l = 0;
+    VALUE *v = 0;

...and tons of similar "fixes". Please see the comments on the lines you are
changing.
WE DO THAT ON PURPOSE: we know that variables are uninitialized. We know it's
ok in these cases.
"int i = i;" trick is used to make compiler stop complaining.

-- 
Configure bugmail: https://bugs.busybox.net/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.