Re: [PATCH] treewide: Dodge dynamic memory allocation in strerror() from glibc > 2.40

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3470 bytes --]

On Wed, Dec 11, 2024 at 01:46:41AM +0100, Stefano Brivio wrote:
> On Wed, 11 Dec 2024 11:26:46 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > On Wed, Dec 11, 2024 at 12:29:09AM +0100, Stefano Brivio wrote:
> > > With glibc commit 25a5eb4010df ("string: strerror, strsignal cannot
> > > use buffer after dlmopen (bug 32026)"), strerror() now needs, at least
> > > on x86, the getrandom() and brk() system calls, in order to fill in
> > > the locale-translated error message. But getrandom() and brk() are not
> > > allowed by our seccomp profiles.
> > > 
> > > This became visible on Fedora Rawhide with the "podman login and
> > > logout" Podman tests, defined at test/e2e/login_logout_test.go in the
> > > Podman source tree, where pasta would terminate upon printing error
> > > descriptions (at least the ones related to the SO_ERROR queue for
> > > spliced connections).
> > > 
> > > Avoid dynamic memory allocation by calling strerrordesc_np() instead,
> > > which is a GNU function returning a static, untranslated version of
> > > the error description. If it's not available, keep calling strerror(),
> > > which at that point should be simple enough as to be usable (at least,
> > > that's currently the case for musl).
> > > 
> > > Reported-by: Paul Holzinger <pholzing@redhat.com>
> > > Link: https://github.com/containers/podman/issues/24804
> > > Analysed-by: Paul Holzinger <pholzing@redhat.com>
> > > Signed-off-by: Stefano Brivio <sbrivio@redhat.com>  
> > 
> > In the short term, this seems like a reasonable fix, except for one
> > detail.  Generally '_' prefixed names are reserved for "the system",
> > meaning libc in practice.  So I don't think __strerror() is a good
> > choice for our interposed function name.  If we can keep the interface
> > the same and use macro shannigans to interpose it would be nice if we
> > can keep it as 'strerror()' through most of the code.  I think this is
> > possible - see the clang tidy workarounds at the bottom of util.h for
> > example.
> 
> It definitely is, it's even simpler than those as we wouldn't even need
> local variables here, but... it's not strerror(), so I don't think we
> should call it strerror().

Fair point.

> 
> > Otherwise I guess 'strerror_()' or 'passt_strerror()', ugly
> > as those are.
> 
> Picked strerror_(), passt_strerror() is way too long.

Ok.

> > Longer term, it's not awesome to be ignoring the locale.  Could we use
> > the XSI compliant strerror_r() version to get translated messages
> > somewhat portably and without allocation?
> 
> I gave that a thought, but the XSI compliant version of strerror_r()
> needs a user-supplied buffer as well, which would make the callers look
> horrible. We could pass in a static buffer otherwise, but then,
> confusingly, our own version/wrapper of strerror_r() wouldn't be
> thread-safe.

True.

> I wonder if ignoring the locale is really that bad. After all, we print
> all the messages in English, without localisation. Printing the error
> description in other languages is arguably inconsistent.

Hm, I guess.  Maybe we can tackle respecting locale for the kernel
library errors when/if we add localisation support for our own
messages.

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]