[usr/src/sbin/newfs_msdos/mkfs_msdos.c:586]: (error) Memory leak: img Source code is if (sigaction(SIGINFO, &si_sa, NULL) == -1) { warn("sigaction SIGINFO"); return -1; }
Yes, this is a leak. This should fix it: $ svn diff sbin/newfs_msdos/mkfs_msdos.c Index: sbin/newfs_msdos/mkfs_msdos.c =================================================================== --- sbin/newfs_msdos/mkfs_msdos.c (revision 291608) +++ sbin/newfs_msdos/mkfs_msdos.c (working copy) @@ -583,6 +583,7 @@ si_sa.sa_handler = infohandler; if (sigaction(SIGINFO, &si_sa, NULL) == -1) { warn("sigaction SIGINFO"); + free(img); return -1; } for (lsn = 0; lsn < dir + (fat == 32 ? bpb.bpbSecPerClust : rds); lsn++) {
Although this fix is not wrong as such, fixing the memory leak in this extremely rare error case does not make much sense if the happy path and error cases that may actually happen still leak the memory. Note that this memory leak is only a problem if the code in newfs_msdos.c is changed so it does not exit immediately after mkfs_msdos() returns, or for static analyzers. The error from sigaction() could also be ignored (silently or loudly), reducing the code size, since a SIGINFO handler is not strictly required for creating a filesystem.
(In reply to Jilles Tjoelker from comment #2) You're right. Upstream (? NetBSD) is just as bad in this regard.
>Although this fix is not wrong as such, fixing the memory leak in this >extremely rare error case does not make much sense if the happy path and error >cases that may actually happen still leak the memory. True, but if there were leaks on the main path, I'd have reported them before a leak on an error path. More detail: $ ~/cppcheck/trunk/cppcheck --enable=all usr/src/sbin/newfs_msdos/mkfs_msdos.c Checking usr/src/sbin/newfs_msdos/mkfs_msdos.c... [usr/src/sbin/newfs_msdos/mkfs_msdos.c:586]: (error) Memory leak: img $ I see no evidence of any other memory leaks, or any other detectable problems, in the source code file. A patch for the leak has been produced, problem closed AFAIK.
Review posted here: https://reviews.freebsd.org/D4405
A commit references this bug: Author: ngie Date: Sun Dec 6 21:07:33 UTC 2015 New revision: 291908 URL: https://svnweb.freebsd.org/changeset/base/291908 Log: Fix leak in mkfs_msdos(..) by initializing img to NULL and free'ing at the end of the function Differential Revision: https://reviews.freebsd.org/D4405 MFC after: 1 week PR: 204943 Reviewed by: emaste, jilles Reported by: David Binderman <dcb314@hotmail.com> Sponsored by: EMC / Isilon Storage Division Changes: head/sbin/newfs_msdos/mkfs_msdos.c
The MFC is contingent on the following revisions being MFCed: r289376, r289424, r291219, r291382.
MFCing this change would require MFCing these changes as well: r276737 r289369 r289376 r289424 r291219 r291382 r291385 291908 r276737 could break applications that use DIOCGDINFO, DIOCSDINFO, DIOCWDINFO, and/or DIOCBSDBB. This change isn't worth MFCing..
A commit references this bug: Author: ngie Date: Sun Dec 13 21:09:47 UTC 2015 New revision: 292168 URL: https://svnweb.freebsd.org/changeset/base/292168 Log: MFC r276737,r289369,r289376,r289424,r291219,r291382,r291385,r291908: r276737 (by imp): Remove old ioctl use and support, once and for all. r289369 (by emaste): newfs_msdos: rework option parsing to match NetBSD NetBSD split newfs_msdos in two so that they can reuse the file system creation part in makefs. This change is a step on the path of bringing that support to FreeBSD. Reviewed by: kib, pfg Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D3905 r289376 (by emaste): newfs_msdos: move mkfs_msdos to separate file for later use in makefs Sponsored by: The FreeBSD Foundation r289424 (by emaste): newfs_msdos: prefer snprintf to sprintf Obtained from: NetBSD Sponsored by: The FreeBSD Foundation r291219 (by emaste): newfs_msdos: rework error handling for eventual use in makefs Return -1 on errors from mkfs_msdos() instead of err()/errx(), to allow different consumers to handle errors as appropriate. Obtained from: NetBSD Sponsored by: The FreeBSD Foundation r291382 (by emaste): mkfs_msdos: sync with NetBSD Add a sanity test and clean up whitespace. Obtained from: NetBSD r291385 (by emaste): Use netbsd usage() implementation in newfs_msdos In r289629 newfs_msdos option descriptions are available in mkfs_msdos.h. Obtained from: NetBSD r291908: Fix leak in mkfs_msdos(..) by initializing img to NULL and free'ing at the end of the function Differential Revision: https://reviews.freebsd.org/D4405 PR: 204943 Reviewed by: emaste, jilles Reported by: David Binderman <dcb314@hotmail.com> Sponsored by: EMC / Isilon Storage Division Changes: _U stable/10/ stable/10/lib/libc/regex/grot/Makefile stable/10/lib/libc/regex/grot/main.c stable/10/lib/libc/regex/grot/split.c
(In reply to commit-hook from comment #9) Please ignore this comment. I accidentally committed the wrong message.