[dm-devel] [PATCH 0/4] remove dangerous cleanup __attribute__ uses

[Martin Wilck]

On Tue, 2022-10-11 at 16:52 -0500, Benjamin Marzinski wrote:
> the cleanup __attribute__ is only run when a variable goes out of
> scope
> normally. It is not run on pthread cancellation. This means that
> multipathd could leak whatever resources were supposed to be cleaned
> up
> if the thread was cancelled in a function using variables with the
> cleanup __attribute__. This patchset removes all these uses in cases
> where the code is run by multipathd and includes a cancellation point
> in the variables scope (usually condlog(), which calls fprintf(), a
> cancellation point, the way multipathd is usually run).

I have to say I don't like this. 

Cleaning up resources is certainly  very important, but in most of
cases it's only about freeing memory on exit: memory which is going to
be freed by the system anyway when multipathd terminates. Resource
cleanup matters much more during runtime than on exit. The only threads
that are cancelled during multipathd runtime are the TUR threads.
It's nice to have valgrind show zero leaks when we kill multipathd out
if the sudden. But we should weigh this against the side effects it
has, which is ugly, hard-to-maintain code.

pthread_cleanup_push()/pop() calls contribute a lot to the bad
readability and maintainability of much of the multipath code base, not
to mention the weird errors some gcc versions throw for
pthread_cleanup() constructs. I'd rather try to get rid of as much of
them as we can. I know it won't be possible everywhere, because some
resources (like file descriptors) really need to be cleaned up, but I
am really unsure whether we need pthread cleanup for every free()
operation.

__attribute__((cleanup())), on the contrary, goes a long way to make
code more readable and easier to review. It actually helps a lot to
ensure resources are properly cleaned up, considering how easily a
"goto cleanup;" statement may be forgotten. Replacing it by
pthread_cleanup() and goto statements is undesirable, IMO.

If your concern is really condlog(), let's discuss if we can find a way
to convert this such that it is no cancellation point any more. I can
imagine converting the locking in log_safe() from a mutex into some
lockless scheme, using atomic variables, and using the log thread not
only for syslog, but also for the fprintf() case.

Regards
Martin


> 
> Benjamin Marzinski (4):
>   libmultipath: don't print garbage keywords
>   libmultipath: avoid STRBUF_ON_STACK with cancellation points
>   libmultipath: use regular array for field widths
>   libmultipath: avoid cleanup __attribute__ with cancellation points
> 
>  libmpathutil/parser.c                    | 13 ++--
>  libmpathutil/strbuf.h                    |  4 +-
>  libmultipath/alias.c                     | 59 ++++++++++-------
>  libmultipath/blacklist.c                 |  4 +-
>  libmultipath/configure.c                 |  6 +-
>  libmultipath/discovery.c                 | 34 ++++++----
>  libmultipath/dmparser.c                  | 23 +++----
>  libmultipath/foreign.c                   |  9 +--
>  libmultipath/generic.c                   | 14 ++--
>  libmultipath/libmultipath.version        |  4 +-
>  libmultipath/print.c                     | 82 ++++++++++++++--------
> --
>  libmultipath/print.h                     |  4 +-
>  libmultipath/prioritizers/weightedpath.c | 22 ++++---
>  libmultipath/propsel.c                   | 76 ++++++++++++++++------
>  libmultipath/sysfs.h                     | 11 +---
>  libmultipath/uevent.c                    |  6 +-
>  multipath/main.c                         |  5 +-
>  multipathd/cli_handlers.c                | 52 +++++++--------
>  multipathd/main.c                        | 49 ++++++++------
>  19 files changed, 286 insertions(+), 191 deletions(-)
>