On Tue, 2025-05-06 at 18:26 -0400, Benjamin Marzinski wrote: > 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_*().
No not all. I double checked quite a few. Maybe I got too sloppy at some point and started dismissing these warnings prematurely. > 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. I've been thinking for some time that we should have a single function vector_alloc_and_set_slot(). > 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. Right. Then we should add assert() statements rather than NULL checks, both when adding slots and when reading from vectors. > 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 Does this mean you intend to work on it and send patches? Regards, Martin > . 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
