I applied a memory leak detection tool, called melton(http://lcs.ios.ac.cn/~xuzb/melton.html), to detect the potential bugs in openssh-5.9p1. The url below is the index of bug reports that are checked as real bugs manually. http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/index.html Does it provide enough info to report bugs . Do I need to add some description for each bug? Hope for your replies!
Thanks. In general the reports look pretty good. I think this one is a false positive: http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/report-sXUkMC.html#EndPath max_fd2 = max_fd; client_wait_until_can_do_something(&readset, &writeset, &max_fd2, &nalloc, rekeying); Allocated memory never released. Potential memory leak Heap object allocated here is not freed The only way out of that loop is if quit_pending is set, after which readset and writeset are freed. I can't figure out what these two are complaining about: http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/report-Fs8fvc.html#EndPath http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/report-u6oVoX.html#EndPath I'll attach a patch for the rest shortly.
Created attachment 2124 [details] fix memory leaks
Comment on attachment 2124 [details] fix memory leaks >+++ readconf.c 30 Dec 2011 09:42:23 -0000 [...] >+ if (arg != NULL) >+ xfree(arg); actually this one is a no-op and is not needed.
http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/report-u6oVoX.html#EndPath This report means that the heap object allocated to fwd.connect_host by function "parse_forward" is not freed at the end of the function since fwd is a local variable. http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/report-Fs8fvc.html#EndPath In this report, the loop below iterates two times. At the first iteration, a heap object is allocated to options->user at "charptr = &options->user; ... *charptr = xstrdup(arg);", which is leaked at the second iteration if options->user is reassigned. while (fgets(line, sizeof(line), f)) { linenum++; if (process_config_line(options, host, line, filename, linenum, &active) != 0) bad_options++; } Here is a list of some bugs, most of which are confirmed as false alarms by myself. It may contain some potential bugs or be helpful with you, although most of these are useless or can be eliminated by improving our tool. http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/falsealarms/index.html
(In reply to comment #4) > http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/report-u6oVoX.html#EndPath > > This report means that the heap object allocated to fwd.connect_host > by function "parse_forward" is not freed at the end of the function > since fwd is a local variable. > > Is this report a real bug? If so, shall we fix this bug? > > http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/realbugs/report-Fs8fvc.html#EndPath > > In this report, the loop below iterates two times. At the first > iteration, a heap object is allocated to options->user at "charptr = > &options->user; ... *charptr = xstrdup(arg);", which is leaked at the > second iteration if options->user is reassigned. > > while (fgets(line, sizeof(line), f)) { > linenum++; > if (process_config_line(options, host, line, filename, linenum, > &active) != 0) > bad_options++; > } > This report may be a false positive as options->user will never be reassigned. > > Here is a list of some bugs, most of which are confirmed as false > alarms by myself. It may contain some potential bugs or be helpful with > you, although most of these are useless or can be eliminated by > improving our tool. > http://lcs.ios.ac.cn/~xuzb/bugsfound/memleak/openssh-5.9p1/falsealarms/index.html
I've committed the following fixes: http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/mux.c.diff?r1=1.42;r2=1.43;f=h http://www.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/sshconnect2.c.diff?r1=1.197;r2=1.198;f=h and I'm working through the rest.
Retarget to openssh-6.4
Retarget 6.3 -> 6.4
Retarget incomplete bugs / feature requests to 6.6 release
Retarget to 6.7 release, since 6.6 was mostly bugfixing.
Remove from 6.6 tracking bug
Retarget incomplete bugs to 6.8 release.
These bugs are no longer targeted at the imminent 6.7 release
OpenSSH 6.8 is approaching release and closed for major work. Retarget these bugs for the next release.
Retarget to 6.9
Comment on attachment 2124 [details] fix memory leaks I think there is nothing left to do here: >--- mux.c 18 Dec 2011 23:52:21 -0000 1.35 >+++ mux.c 30 Dec 2011 09:19:51 -0000 All committed >Index: readconf.c >=================================================================== >RCS file: /home/dtucker/openssh/cvs/openssh/readconf.c,v >retrieving revision 1.174 >diff -u -p -r1.174 readconf.c >--- readconf.c 2 Oct 2011 07:59:03 -0000 1.174 >+++ readconf.c 30 Dec 2011 09:42:23 -0000 >@@ -1063,6 +1063,8 @@ parse_int: > fatal("%.200s line %d: garbage at end of line; \"%.200s\".", > filename, linenum, arg); > } >+ if (arg != NULL) >+ xfree(arg); > return 0; This is incorrect - arg comes from strdelim here and is a pointer somewhere inside the line being parsed. It cannot be freed itself. >--- sshconnect2.c 29 May 2011 11:42:34 -0000 1.180 >+++ sshconnect2.c 30 Dec 2011 09:27:33 -0000 >@@ -1323,8 +1323,11 @@ load_identity_file(char *filename) > return NULL; > } > private = key_load_private_type(KEY_UNSPEC, filename, "", NULL, &perm_ok); >- if (!perm_ok) >+ if (!perm_ok) { >+ if (private != NULL) >+ key_free(private); This code has been refactored and the leak eliminated. >@@ -1892,9 +1895,9 @@ authmethod_get(char *authlist) > xfree(name); > return current; > } >+ if (name != NULL) >+ xfree(name); This is already there.
Set all RESOLVED bugs to CLOSED with release of OpenSSH 7.1