Bug 203649
| Summary: | makefs: Coverity CID 1305659: Unclear whether reaction on malloc failure suffices. | ||
|---|---|---|---|
| Product: | Base System | Reporter: | scdbackup |
| Component: | bin | Assignee: | Enji Cooper <ngie> |
| Status: | Closed FIXED | ||
| Severity: | Affects Some People | CC: | ngie |
| Priority: | --- | Keywords: | patch |
| Version: | CURRENT | ||
| Hardware: | Any | ||
| OS: | Any | ||
--------------- Source analysis: I meanwhile learned that err() is also available in userspace and indeed exits its process. So it is not a potential SIGSEGV but just a surplus test. --------------- Remedy proposal: - if (var) - free(var); + free(var); Hmm, this is in bin, not kern. Oops. We've diverged a lot from upstream. Need to add emalloc to libc or libnetbsd to pull in the next set of changes from upstream for makefs. The 2-liner looks good though. I'll commit it to fix the Coverity issue. Thanks for looking into this issue. There is an upstream ? (Normally that's my role, but here i am friendly competitor.) Shall i put my findings there ? (PR 203531, 203644 to 203648) Please have a look at PR 203648 which i deem as obvious and simple as this one. A commit references this bug: Author: ngie Date: Mon Oct 19 18:45:14 UTC 2015 New revision: 289601 URL: https://svnweb.freebsd.org/changeset/base/289601 Log: Don't check if `val` is NULL before calling free; free(3) already handles this MFC after: 1 week PR: 203649 Submitted by: Thomas Schmitt <scdbackup@gmx.net> Coverity CID: 1305659 Sponsored by: EMC / Isilon Storage Division Changes: head/usr.sbin/makefs/cd9660.c (In reply to scdbackup from comment #4) I looked through the code and it's been completely refactored as of 2 years ago: http://cvsweb.netbsd.org/bsdweb.cgi/src/usr.sbin/makefs/cd9660.c?rev=1.39&content-type=text/x-cvsweb-markup&only_with_tag=MAIN Well, i still find all four bugs of PR 203531 in NetBSD CVS. (Those flaws in FreeBSD 11 installation ISOs brought me to freebsd-hackers and to Coverity checks of makefs.) Is there some established way to share bugs between both projects ? A commit references this bug: Author: ngie Date: Mon Oct 26 03:53:49 UTC 2015 New revision: 289989 URL: https://svnweb.freebsd.org/changeset/base/289989 Log: MFC r289601: Don't check if `val` is NULL before calling free; free(3) already handles this PR: 203649 Submitted by: Thomas Schmitt <scdbackup@gmx.net> Coverity CID: 1305659 Sponsored by: EMC / Isilon Storage Division Changes: _U stable/10/ stable/10/usr.sbin/makefs/cd9660.c A commit references this bug: Author: ngie Date: Mon Oct 26 03:55:13 UTC 2015 New revision: 289990 URL: https://svnweb.freebsd.org/changeset/base/289990 Log: MFstable/10 r289989: MFC r289601: Don't check if `val` is NULL before calling free; free(3) already handles this PR: 203649 Submitted by: Thomas Schmitt <scdbackup@gmx.net> Coverity CID: 1305659 Sponsored by: EMC / Isilon Storage Division Changes: _U stable/9/ _U stable/9/usr.sbin/ _U stable/9/usr.sbin/makefs/ stable/9/usr.sbin/makefs/cd9660.c |
usr.sbin/makefs/cd9660.c CID 1305659: Dereference before null check (REVERSE_INULL)i check_after_deref: Null-checking var suggests that it may be null, but it has already been dereferenced on all paths leading to the check. 431 if (var) 432 free(var); --------------- Source analysis: Indeed the function should bail out when allocation fails. 327 if ((var = strdup(option)) == NULL) 328 err(1, "allocating memory for copy of option string"); If err() does not finally call exit(), then the program runs into a SIGSEGV by 331 val = strchr(var, '='); The function cd9660_parse_opts() gets called by usr.sbin/makefs/makefs.c if (! fstype->parse_options(p, &fsoptions)) usage(); usage() calls exit(1). So i assume that return NULL would be the way to indicate error and cause abort. But usage() will indicate a user error where a resource shortage is the reason. --------------- Remedy proposal: Call exit(1) if no memory is available. (Unless you can find the definition of err() and verify that it calls exit().) - if ((var = strdup(option)) == NULL) + if ((var = strdup(option)) == NULL) { err(1, "allocating memory for copy of option string"); + exit(1); + } In any case remove the test which made Coverity suspicious. - if (var) - free(var); + free(var);