[dm-devel] [PATCH 0/6] libmultipath: check udev* func return value

[Martin Wilck]

On Thu, 2020-09-17 at 21:26 -0500, Benjamin Marzinski wrote:
> On Tue, Sep 15, 2020 at 12:38:27PM +0800, lixiaokeng wrote:
> > Hi,
> >   The udev* function may return NULL,and it will be
> > dereferenced in str* and sscanf func. For example,
> > there is a coredump caused in add func, which show in
> > be7a043(commit id) in upstream-queue. We check the
> > return value to avoid dereference NULL.
> > 
> > repo: openSUSE/multipath-tools
> > repo link: https://github.com/openSUSE/multipath-tools
> > branch: upstream-queue
> > 
> For the set
> Reviewed-by: Benjamin Marzinski <bmarzins at redhat.com>

Well, the set got the logic wrong in 3 out of 6 patches :-)

In general, as these fixes are all very similar, I would prefer
bundling them together into one patch, and I would like to make sure
(and assert in the commit message) that the set fixes
*all* possible NULL pointer dereferences related to accessing udev
properties (I haven't checked whether this is the case for the set).

Also, while I'd ack this set if the logic was correct, I think that in
a way it's papering over the actual problem. udev_device_get_XYZ()
functions fail *only* if the underlying sysfs directory has vanished
(well, perhaps for out-of-memory, too, but let's put that aside for a
moment). It that case, we should actually not just return an error code
- we should realize that there's a problem with the device (it probably
has been deleted from the system and shouldn't be used any more in any
way), and that we need to deal with it somehow. This is a fundamental
problem for all user space programs that use udev_devices for more than
a few CPU cycles. That doesn't mean the set is wrong, but we should
keep this in mind. I have something cooking for a more complete
solution, which isn't ready yet.

Finally, @lixiaokeng, please check your inbox once more. You still
haven't fixed #11/14 of your previous series.

Regards,
Martin