RFC on inline wrappers and symbol poisoning (was: RFC on enforcing best-practice through wrappers? [PR15819, PR15722])

[Alexandre Oliva <aoliva at redhat dot com>]

I'd like to single out one point from the previous discussion to
elaborate on and attempt to reach consensus on policy regarding
poisoning symbols to enforce best practice:

On Nov  5, 2014, Alexandre Oliva <aoliva@redhat.com> wrote:

> There are various ways to go about fixing this, ranging from using
> __poll and __socket internal wrappers/aliases to avoid the undesirable
> problems in the first place (say, having __poll never return EINTR, but
> retrying with a recomputed timeout instead instead, and having __socket
> always set CLOEXEC), introducing alternate functions to do so and
> perhaps poisoning the original symbols so that misuses are flagged, up
> to just offering wrappers that implement the best practice in internal
> header wrappers or new internal headers.  AFAICT we don't seem to have
> any recommended practice on how to factor out, let alone internally
> enforce, this sort of best practice.

Having socket always set CLOEXEC turned out to be a bad example, because
in some cases we want to create sockets that do not have CLOEXEC set.

This might seem like an argument against poisoning the socket symbols to
avoid their internal use in glibc, but maybe we'd be better off changing
them to *another* wrapper that indicates whether the socket they create
is internal or not through e.g. passing SOCK_CLOEXEC in the flags, so
that the wrapper can fallback to __socket_cloexec if the flag is set.

IIRC there are only uses in sunrpc that might have benefitted from this,
but if we wanted to enforce an explicit decision on whether or not we
want SOCK_CLOEXEC at each socket creation point, poisoning the __socket
and socket tokens, and introducing such wrappers may be a good way to
implement it.

The ultimate goal is that, at some point, no explicit socket calls would
remain, and at that point we will know that for every socket call the
decision on whether or not to use SOCK_CLOEXEC was thought about and
made, rather than the current state, in which there are plenty of socket
calls that each require some (occasionally non-trivial) analysis to
figure out whether or not they should have the SOCK_CLOEXEC flag set.
We don't want to have to repeat such analysis over and over and over.

Of course, just adding comments is a way to avoid costly analysis, but
in general the comments won't be at the same line as the call, so at
least some time will be wasted after a global grep for unadorned calls.
Using wrappers that indicate the analysis was done before avoids that.


Now, another issue is that so far the proposed wrappers have been in
separate headers, so that their use (and the symbol poisoning that might
come with them) has been voluntary.

In order to enforce best practice and informed decisions all over glibc,
this is not enough.  We'd have to have the wrappers in the internal
headers that are already used all over to get a declaration for the
wrapped functions in the first place.  Without that, working around
policy becomes as simple as refraining from opting into the
policy-enforcing headers.


So here's what I propose:

Introduce internal header include/poll-noeintr.h, with the
__poll_noeintr inline wrapper function that retries if interrupted by
signals, and #pragma GCC poison poll __poll, so that all uses of poll
and __poll will be flagged and replaced with __poll_noeintr.  Then,
include poll-noeintr.h at the end of sys/poll.h, so that the use of
poll_noeintr is enforced all over.

Introduce internal header include/socket-cloexec.h, with
__socket_cloexec and __socket_xflag inline wrapper functions, so that we
can poison socket and __socket, so that all uses of the poisoned symbols
have to call either __socket_cloexec, for sockets that are definitely
internal to libc and that therefore should ideally have the CLOEXEC flag
set, or __socket_xflag, that explicitly flags whether the socket is
internal or external by passing it the SOCK_CLOEXEC flag.  Then, include
socket-cloexec.h in sys/socket.h, so that uses of __socket that have not
been replaced with either __socket_cloexec or __socket_xflag are flagged
and fixed.

In general, whenever we want to enforce internally one particular use
pattern for a function, introduce a header that introduces one or more
wrappers for that function, covering all sorts of internal uses and
poisoning the function's original symbols.  Then, include this header at
the end of the internal header that declares the original symbols, so
that remaining uses are flagged and the policy is enforced all over.


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist|Red Hat Brasil GNU Toolchain Engineer