Found some different code miss using a static code analysis program called cppcheck. Among other things, failed to release the memory and file resources, possible to use an uninitialized variable and some printf with wrong unsigned/signed characters. And more... Attacht is the cppcheck log file.
Cppcheck download from: http://sourceforge.net/projects/cppcheck/ Use option like: cppcheck --force --quiet --enable=all . Not that file util-linux/fsck_minix.c and util-linux/mkfs_minix.c makes Cppcheck loop and eat memory. Easiest fix remove them.
Created attachment 5126 [details] Suggestions for a bug fix I have made a fix patch for this, it's based on master 72745632a13ccd12232127b31e1656f2f7ebcaff Hope it's ok, but a code verification surely needed. (Should "Reassignment" then be with me?)
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.
Fixed in git. Thanks!
(In reply to comment #4) > Fixed in git. Thanks! Than you for the great busybox project :)
Hi I want to begin by thanking you for busybox , and it's fun to be able to contribute to such a great project :) But some commentators , I feel is in place.. (In reply to comment #3) > 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. Although the program is closed and we do not "have to" close the file pointer, we should surely do so when we found the "error"... I think. > --- a/networking/udhcp/d6_dhcpc.c > +++ b/networking/udhcp/d6_dhcpc.c > @@ -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. I know you can make free ( NULL) now, but actually thought that this could be a problem on older systems , so this was an attempt on my part to be a little backwards compatible .. But after a bit of googling I have not found anything to suggest that this ever been a problem. Nice, then I learned something :) > - 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. I know char * X = X; allows variable is uninitialized while eliminating warnings from gcc. The question is whether this is worth doing so, in pure assembler code it saves one command ..! Is that the optimization degree busybox tend? There is for example, MISRA C standard where one should always put all the variables to an initial value because this is one of the major sources of error in C. Not least when one gradually make even more changes to existing code. Or another way to look at this. If there be only one of these values, which were the slightest chance of being used uninitialized it would not be worth this minimal optimization ...? I've found 20, this was why I start to fix all of them in the first place. coreutils/expr.c : 326] : (error) Uninitialized variable : l coreutils/expr.c : 327] : (error) Uninitialized variable : v coreutils/test.c : 633] : (error) Uninitialized variable : i miscutils/chrt.c : 65] : (error) Uninitialized variable : priority miscutils/taskset.c : 89] : (error) Uninitialized variable : aff miscutils/ubi_tools.c : 109] : (error) Uninitialized variable : size_bytes networking/httpd.c : 2469] : (error) Uninitialized variable : server_socket networking/nc_bloaty.c : 590] : (error) Uninitialized variable : zp networking/nc_bloaty.c : 591] : (error) Uninitialized variable : np networking/tcpudp.c : 210] : (error) Uninitialized variable : len_per_host networking/tcpudp.c : 217] : (error) Uninitialized variable : remote_hostname networking/tcpudp.c : 218] : (error) Uninitialized variable : remote_addr networking/udhcp/d6_dhcpc.c : 1059] : (error) Uninitialized variable : timestamp_before_wait networking/udhcp/dhcpc.c : 1399] : (error) Uninitialized variable : timestamp_before_wait networking/udhcp/domain_codec.c : 33] : (error) Uninitialized variable : ret networking/zcip.c : 160] : (error) Uninitialized variable : addr shell/hush.c : 4954] : (error) Uninitialized variable : exp_save shell/hush.c : 4956] : (error) Uninitialized variable : exp_word util-linux/fbset.c : 421] : (error) Uninitialized variable : mode util-linux/mdev.c : 624] : (error) Uninitialized variable : aliaslink