On Mon, May 05, 2025 at 06:29:52PM +0200, Martin Wilck wrote: > Xose has kindly informed me about the warnings from Fedora's static > code analysis tool. > > I'm sending a couple of related fixes here. Most of the warnings > appear to be false positives though, see below (a second look by > another eye pair would be appreciated).
For all but patch 4/6: Reviewed-by: Benjamin Marzinski <bmarz...@redhat.com> > > Regards > Martin > > On Sat, 2025-05-03 at 23:29 +0200, Xose Vazquez Perez wrote: > > Findings by static analyzers in Fedora 43 Critical Path Packages: > > https://marc.info/?l=fedora-devel-list&m=174582743823723 > > > > device-mapper-multipath-0.11.1-1.fc43: > > https://openscanhub.fedoraproject.org/task/51915/ > > > > device-mapper-multipath-0.11.1-1.fc43 List of Findings: > > https://openscanhub.fedoraproject.org/task/51915/log/device-mapper-multipath-0.11.1-1.fc43/scan-results.html > > I'll leave the shellcheck errors in mpathconf to Ben. > > > Error: GCC_ANALYZER_WARNING (CWE-775): [#def42] > > multipath-tools-0.11.1/kpartx/dasd.c:89:24: warning[-Wanalyzer-fd-leak]: > > leak of file descriptor ‘fd_dasd’ > > multipath-tools-0.11.1/kpartx/dasd.c:69:1: enter_function: entry to > > ‘read_dasd_pt’ > > ... > > multipath-tools-0.11.1/kpartx/dasd.c:134:20: branch_true: following ‘true’ > > branch (when ‘fd_dasd < 0’)... > > branch_true: ...to here > > This one looks like a false positive to me. If fd_dasd < 0, it can't be > leaked. > > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def45] > > multipath-tools-0.11.1/libmpathutil/parser.c:139:29: > > warning[-Wanalyzer-null-dereference]: dereference of NULL ‘keyword’ > > False positive, too. VECTOR_SLOT won't return NULL here. > We could add a test to make the compiler happy, but I don't quite see the > point. > > > Error: GCC_ANALYZER_WARNING (CWE-457): [#def49] > > multipath-tools-0.11.1/libmultipath/alias.c:201:31: > > warning[-Wanalyzer-use-of-uninitialized-value]: use of uninitialized value > > ‘bdg’ > > False positive. bdg won't be NULL here. > > > Error: GCC_ANALYZER_WARNING (CWE-465): [#def50] > > multipath-tools-0.11.1/libmultipath/dmparser.c:31:12: > > warning[-Wanalyzer-deref-before-check]: check of ‘p’ for NULL after already > > dereferencing it > > False positive. *dst is non-NULL when we call merge_words(). We must > double-check > after calling realloc(). > (That said, we should reimplement this using strbuf functions). > > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def51] > > multipath-tools-0.11.1/libmultipath/dmparser.c:436:25: > > warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pgp’ > > False positive. VECTOR_SLOT won't return NULL here. Same for the next 3 > errors. > > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def55] > > multipath-tools-0.11.1/libmultipath/dmparser.c:501:33: > > warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp’ > > Likewise. Same for the next 2 errors. > > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def58] > > multipath-tools-0.11.1/libmultipath/foreign/nvme.c:494:28: > > warning[-Wanalyzer-null-dereference]: dereference of NULL ‘path’ > > The nvme_pathgroup is a member of struct nvme_path. nvme_pg_to_path() doesn't > return NULL. Same for the next 2 errors in nvme.c > > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def62] > > multipath-tools-0.11.1/libmultipath/pgpolicies.c:191:17: > > warning[-Wanalyzer-null-dereference]: dereference of NULL ‘pp1’ > > Again, false positives; VECTOR_SLOT() doesn't return NULL here. > Same for the next 3 errors. > > > Error: GCC_ANALYZER_WARNING (CWE-401): [#def66] > > multipath-tools-0.11.1/libmultipath/print.c:920:16: > > warning[-Wanalyzer-malloc-leak]: leak of ‘alloc_path_layout()’ > > False positive. Strange one - gcc doesn't understand its own > __attribute__((cleanup))? > Or am I blind? Same for the next one. > > > Error: GCC_ANALYZER_WARNING (CWE-126): [#def70] > multipath-tools-0.11.1/libmultipath/uevent.c:122:9: > warning[-Wanalyzer-out-of-bounds]: stack-based buffer over-read > > This one is somewhat harder because it involves list handling macros, but I > believe that our code is correct and that this is a false positive, too. > > > Error: GCC_ANALYZER_WARNING (CWE-775): [#def69] > > multipath-tools-0.11.1/libmultipath/sysfs.c:309:22: > > warning[-Wanalyzer-fd-leak]: leak of file descriptor ‘open(&pathbuf, 0)’ > > Again, false positive; there's a cleanup function for fd. > > > Error: GCC_ANALYZER_WARNING (CWE-775): [#def71] > > multipath-tools-0.11.1/libmultipath/valid.c:208:34: > > warning[-Wanalyzer-file-leak]: leak of FILE ‘fopen(&mountinfo, "r")’ > > False positive, gcc doesn't understand the cleanup_fclose(). > Same for the next error. > > > Error: GCC_ANALYZER_WARNING (CWE-415): [#def73] > > multipath-tools-0.11.1/multipathd/fpin_handlers.c:508:17: > > warning[-Wanalyzer-double-free]: double-‘free’ of > > ‘els_marginal_list_head.next + -2056’ > > I don't see an error in this code. Looks like another false positive to me. > > > Error: GCC_ANALYZER_WARNING: [#def74] > > multipath-tools-0.11.1/multipathd/fpin_handlers.c:624:23: > > warning[-Wanalyzer-fd-use-without-check]: ‘read’ on possibly invalid file > > descriptor ‘fd’ > > False positive, too. fd == -1 is checked beforehand. > > > Error: GCC_ANALYZER_WARNING (CWE-688): [#def75] > > multipath-tools-0.11.1/multipathd/main.c:3286:21: > > warning[-Wanalyzer-null-argument]: use of NULL ‘new’ where non-null expected > > Similar to other false positives above, VECTOR_SLOT() won't return NULL in a > loop like this. > > > Error: GCC_ANALYZER_WARNING (CWE-775): [#def77] > > multipath-tools-0.11.1/multipathd/main.c:3973:12: > > warning[-Wanalyzer-fd-leak]: leak of file descriptor > > ‘dup2(open("/dev/null", 2), 0)’ > > Yet another false positive, the fd is closed in the cleanup handler. Same for > the next 3 errors. > > > Error: GCC_ANALYZER_WARNING (CWE-476): [#def81] > multipath-tools-0.11.1/multipathd/multipathc.c:97:30: > warning[-Wanalyzer-null-dereference]: dereference of NULL ‘kw’ > > Another case of VECTOR_SLOT() false positive. Same for the next error. > > Regards > Martin > > Martin Wilck (6): > kpartx_id: fix shellcheck reported errors > kpartx: fix file descriptor leak > libmpathpersist: fix memory leak in mpath_prout_rel() > libmpathutil: silence compiler warning in vector_del_slot() > libmultipath: fix undefined behavior in 31-bit shift > libmultipath: prioritizers/iet: fix possible NULL dereference > > kpartx/kpartx.c | 14 +++++++------- > kpartx/kpartx_id | 8 ++++---- > libmpathpersist/mpath_persist_int.c | 12 ++++++------ > libmpathutil/vector.c | 16 ++++++++-------- > libmultipath/nvme/nvme-ioctl.c | 2 +- > libmultipath/prioritizers/iet.c | 3 +++ > 6 files changed, 29 insertions(+), 26 deletions(-) > > -- > 2.49.0
