Re: [PATCH] Preload infrastructure.

["Joseph S. Myers" <joseph at codesourcery dot com>]

On Tue, 22 Oct 2013, Ondrej Bilka wrote:

> On Tue, Oct 22, 2013 at 02:56:08PM +0000, Joseph S. Myers wrote:
> > On Mon, 21 Oct 2013, Ondrej Bilka wrote:
> > 
> > > OK to commit?
> > 
> > Please don't ask that for something that is clearly nowhere near ready - 
> > for example, doesn't integrate at all into the glibc build system, doesn't 
> 
> A purpose of initial patch is get framework on which we build. Insisting

It can be a framework in that it only adds one or a few functions, with 
more to be added lately.  But the code added still needs to follow the 
normal coding standards and the contribution checklist.

https://sourceware.org/glibc/wiki/Style_and_Conventions
https://sourceware.org/glibc/wiki/Contribution%20checklist

> on adding functionality only when it is perfect would cause harder
> reviewing of incremental improvements as entire patch should be send,
> making tracking of what is done harder.

Any public interface needs to be supported indefinitely.  We need to make 
sure that public interfaces are well-designed before adding them.  (The 
requirements for supporting a preloaded library are less strong than for 
an exported shared library symbol, but still work on the presumption that 
any interface added is fixed and permanent.)

> >hardcodes libc.so.6 rather than adapting for different SONAMEs on different targets, 
> 
> Elaborate.

alpha uses libc.so.6.1.  Hurd uses libc.so.0.3.  Use the gnu/lib-names.h 
header to get the correct name.  I'm not sure why you need this anyway; 
libmemusage uses RTLD_NEXT rather than explicitly opening libc.

> > introduces an obvious security hole (and > code that won't work on multi-user systems) by hardcoding a path in /tmp, 
> 
> Elaborate why this is security hole.

Because someone could make that name into a symlink and so cause another 
user, when using this functionality, to modify a file they didn't intend 
to modify.

> > fails to use symbol versioning / otherwise ensure that the library only 
> > exports the desired symbols and no others, 
> 
> This library is meant to be used by LD_PRELOAD, elaborate what problems do you
> solve by versioning?

Ensuring that it can only ever override the symbols it's meant to 
override, with everything else being purely internal to the library.

A better answer would be "existing preloaded libraries such as libSegFault 
don't use symbol versioning" - which I think *is* a sufficient reason not 
to need it in a new library, if you carefully model the build and 
installation process for this library on libSegFault and other such 
libraries so it's clear you're not doing anything special and wrong, just 
doing the same as other glibc libraries.  (And then if those libraries get 
fixed to use symbol versioning, as I think they should, this one would 
automatically benefit.)

> >isn't thread-safe (look at all
> > the use of static variables in strncat where the code could end up using 
> > some variables from one thread and some from another),
> 
> That is heuristic to detect quadratic behaviour. For you I will change
> writes to atomic writes and it will work as it is, in multithreaded case
> worst that could happen is that log message will be printed twice.

It should use thread-local storage, so the detection is independent in 
each thread.

> > has no copyright /  license notices, is not documented in the glibc manual, ....
> 
> Could you remove directories benchtests and nptl. These are not
> documented in manual and should be for such serious ommission deleted.

benchtests is not user-visible - we're talking about public interfaces 
being provided for the users of the library.  Again, *think of the 
audience of a feature*.

Development practices were different when NPTL was added.  Note that now 
we expect people adding new functions to it to provide documentation for 
those functions, even though the bulk of the existing library is still 
undocumented.

When you think more carefully about audiences, it becomes clear that the 
whole name of this feature (and the subject line for this email thread) is 
wrong.  libSegFault is preloaded - it's not libc_preload.  libpcprofile is 
preloaded - it's not libc_preload.  The corresponding scripts are 
catchsegv.sh and xtrace.sh, not use_preload.  The fact that something uses 
LD_PRELOAD is an *implementation detail* and not something users should 
need to be concerned about; no one use of preloading should be specially 
distinguished as libc_preload.  The shared object for checking library 
function usage should be named in a way that makes clear *what it aims to 
do* rather than *how it is used*; likewise any wrapper shell script.  And 
since there are two distinct uses here - checking for incorrect usage, and 
suggesting optimizations - I think there should be two separate shared 
objects and shell scripts (though it's certainly a good idea if it's 
possible to use several of these shared objects together).

So: put this code in the debug/ directory like other debugging preloadable 
libraries; split it up by the different things it's usable to debug, so 
each installed shared object does one clearly defined thing and does it 
well; model the build/install system on how other such libraries are 
built/installed; model other things on those libraries as well (such as 
using an environment variable to specify an output file, not a hardcoded 
/tmp path); generally follow established coding practices for such 
libraries.

Given that, I think the principle of support for such checks makes sense - 
*provided* sufficient justification can be provided in an analysis of 
prior art for advantages of your version over previous implementations of 
such features (Jeff Law mentioned memstomp, so your write-up should 
explain what that is, how it works and what the advantages and 
disadvantages of the two approaches are; likewise for any other similar 
tools).

-- 
Joseph S. Myers
joseph@codesourcery.com