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).
I mostly agree with you, except that I wouldn't call all the errors related to VECTOR_SLOT() returning NULL false postitives (so maybe I mostly disagree with you. I didn't count). If we are in a vector_foreach_slot_*() loop and we are looking at VECTOR_SLOT(<vector_we_are_looping_over>, <current_index_value>), then I agree, we can't see a NULL value. But most, if not all, of the "NULL value from a vector" errors that got flagged here were not protected by the checks in vector_foreach_slot_*(). Now I don't know of any cases where we do have NULLs in a vector, but we certainly could. We manipulate them all over the code. If we ever call vector_alloc_slot() and not vector_set_slot() we would have a NULL. There's also no check to make sure that we don't call vector_set_slot() with a NULL value. We do have checks for NULL values when looping through vectors, and we even have vector_repack() to get rid of them. So, it's completely reasonable that a static analyzer would think that we might have a NULL value in a vector. And I'm pretty sure that NULL values in a vector are always a bug. They make vector_foreach_slot() unable to access any values past them, despite the fact that we can keep adding values to the vector. Actually, cleaning this up, so that the code keeps us from having NULL values in vectors as long as we only manipulate them with the functions in vector.h has been on my todo list for a bit. It would take some code auditing to make sure that there isn't a case where we do use vectors where NULL values are expected. But if there is, I think that should use a seperate set of functions, to clearly mark it out as special. -Ben > > 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
