Re: [PATCH 0/6] multipath-tools: static analyzer fixes

[Benjamin Marzinski <bmarzins@redhat.com>]

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 <bmarzins@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