patch: make the size of errbuf configurable

patch: make the size of errbuf configurable

[Z. Gilboa]

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

Greetings,

When a shared library that resides in a deeply nested folder contains 
unresolved (long-named, mangled) symbols, the displayed name of the 
library and/or symbol might get truncated.  The attached patch makes the 
size of errbuf (ldso/dynlink.c) configurable (--with-ld-errbuf-size), 
while yet leaving the default size of 128 unaffected.

Best regards,
Zvi


[-- Attachment #2: ld_errbuf_size.patch --]
[-- Type: text/x-patch, Size: 2139 bytes --]

From 03e738888958d04b6c1bb900d571fb342916bc84 Mon Sep 17 00:00:00 2001
From: Z. Gilboa <zg7s@virginia.edu>
Date: Sun, 19 May 2013 15:33:09 -0400
Subject: [PATCH] ld: errbuf: make the buffer size configurable 	
modified:	configure 	
modified:	src/ldso/dynlink.c

---
 configure          |    9 +++++++++
 src/ldso/dynlink.c |    6 +++++-
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/configure b/configure
index 96f93b2..b063d37 100755
--- a/configure
+++ b/configure
@@ -30,6 +30,9 @@ Optional features:
   --disable-shared        inhibit building shared library [enabled]
   --disable-static        inhibit building static library [enabled]
 
+Configurable settings:
+  --with-ld-errbuf-size   set the size of the dynamic loader's error buffer [128]
+
 Some influential environment variables:
   CC                      C compiler command [detected]
   CFLAGS                  C compiler flags [-Os -pipe ...]
@@ -120,6 +123,7 @@ case "$arg" in
 --disable-warnings|--enable-warnings=no) warnings=no ;;
 --enable-gcc-wrapper|--enable-gcc-wrapper=yes) wrapper=yes ;;
 --disable-gcc-wrapper|--enable-gcc-wrapper=no) wrapper=no ;;
+--with-ld-errbuf-size=*) ld_errbuf_size=${arg#*=} ;;
 --enable-*|--disable-*|--with-*|--without-*|--*dir=*|--build=*) ;;
 --host=*|--target=*) target=${arg#*=} ;;
 -* ) echo "$0: unknown option $arg" ;;
@@ -310,7 +314,12 @@ test -z "$LIBCC" && try_libcc=`$CC -print-file-name=libpcc.a 2>/dev/null` \
                  && tryldflag LIBCC "$try_libcc"
 printf "using compiler runtime libraries: %s\n" "$LIBCC"
 
+# configurable settings
+if [ "$ld_errbuf_size"x != x ]; then
+	CFLAGS="$CFLAGS -DLD_ERRBUF_SIZE=$ld_errbuf_size"
+fi
 
+# generate config.mak
 printf "creating config.mak... "
 
 exec 3>&1 1>config.mak
diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
index dec9511..59106c5 100644
--- a/src/ldso/dynlink.c
+++ b/src/ldso/dynlink.c
@@ -21,8 +21,12 @@
 #include "pthread_impl.h"
 #include "libc.h"
 
+#ifndef LD_ERRBUF_SIZE
+#define LD_ERRBUF_SIZE 128
+#endif
+
 static int errflag;
-static char errbuf[128];
+static char errbuf[LD_ERRBUF_SIZE];
 
 #ifdef SHARED
 
-- 
1.7.9.5

Re: patch: make the size of errbuf configurable

[Rich Felker]

On Sun, May 19, 2013 at 04:12:58PM -0400, Z. Gilboa wrote:
> Greetings,
> 
> When a shared library that resides in a deeply nested folder
> contains unresolved (long-named, mangled) symbols, the displayed
> name of the library and/or symbol might get truncated.  The attached
> patch makes the size of errbuf (ldso/dynlink.c) configurable
> (--with-ld-errbuf-size), while yet leaving the default size of 128
> unaffected.

If at some point we do have build configurations, I'd want it to be
more structured, and for the user-visible names not to reflect
internal variable names, but something more meaningful to users.

With that said, in this case I think a different solution is called
for, mainly because having configurability for such small details is
setting a precedent that, if followed, could lead to hundreds of
trivial configurable knobs and the same testability nightmare that
plagues uClibc.

My preference is that either long pathnames should be truncated in a
reasonable way (keep in mind that the message is *not* parsable by the
caller; the only way it can be used is presenting it to the user) or
larger buffers should be dynamically allocated. The only reason I did
not go the dynamic-allocation path to begin with was to make it so
non-thread-safe usage of dlerror yields (at worst) corrupted messages
rather than crashes. This can also be achieved with dynamic allocation
(as long as the old too-short buffer is never freed), but it's more
complex.

I'd welcome input on which approach users would prefer.

Rich

Re: patch: make the size of errbuf configurable

[Szabolcs Nagy]

* Z. Gilboa <zg7s@eservices.virginia.edu> [2013-05-19 16:12:58 -0400]:
> When a shared library that resides in a deeply nested folder
> contains unresolved (long-named, mangled) symbols, the displayed
> name of the library and/or symbol might get truncated.  The attached
> patch makes the size of errbuf (ldso/dynlink.c) configurable
> (--with-ld-errbuf-size), while yet leaving the default size of 128
> unaffected.

i dont think this is the best place to start adding configurations

i'd prefer simply increasing the buffer if this is an issue

c++ mangled names can get pretty insane, i just checked my system

libstdc++ 128 char:
_ZNKSt8time_getIwSt19istreambuf_iteratorIwSt11char_traitsIwEEE21_M_extract_via_formatES3_S3_RSt8ios_baseRSt12_Ios_IostateP2tmPKw

libicui18n 136 chars:
_ZN7icu_4_218DateIntervalFormat24createSDFPatternInstanceERKNS_13UnicodeStringERKNS_6LocaleEPNS_24DateTimePatternGeneratorER10UErrorCode

libLLVM 334 chars:
_ZN4llvm13LiveIntervals28rewriteInstructionsForSpillsERKNS_12LiveIntervalEbRPKNS_9LiveRangeEPNS_12MachineInstrES9_jibbbbRNS_10VirtRegMapEPKNS_19TargetRegisterClassERNS_11SmallVectorIiLj4EEEPKNS_15MachineLoopInfoERNS_9BitVectorERNS_8DenseMapIjSt6vectorINS0_6SRInfoESaISP_EENS_12DenseMapInfoIjEENSS_ISR_EEEESM_SW_RNSN_IjjST_ST_EERSO_IPS1_SaISZ_EE

Re: patch: make the size of errbuf configurable

[Luca Barbato]

On 05/19/2013 11:05 PM, Szabolcs Nagy wrote:
> * Z. Gilboa <zg7s@eservices.virginia.edu> [2013-05-19 16:12:58 -0400]:
>> When a shared library that resides in a deeply nested folder
>> contains unresolved (long-named, mangled) symbols, the displayed
>> name of the library and/or symbol might get truncated.  The attached
>> patch makes the size of errbuf (ldso/dynlink.c) configurable
>> (--with-ld-errbuf-size), while yet leaving the default size of 128
>> unaffected.
> 
> i dont think this is the best place to start adding configurations
> 
> i'd prefer simply increasing the buffer if this is an issue
> 
> c++ mangled names can get pretty insane, i just checked my system
> 

check boost-spirit for the most amazing results (we are talking
kilobytes for a single symbol)

lu

Re: patch: make the size of errbuf configurable

[Rich Felker]

On Sun, May 19, 2013 at 11:36:05PM +0200, Luca Barbato wrote:
> On 05/19/2013 11:05 PM, Szabolcs Nagy wrote:
> > * Z. Gilboa <zg7s@eservices.virginia.edu> [2013-05-19 16:12:58 -0400]:
> >> When a shared library that resides in a deeply nested folder
> >> contains unresolved (long-named, mangled) symbols, the displayed
> >> name of the library and/or symbol might get truncated.  The attached
> >> patch makes the size of errbuf (ldso/dynlink.c) configurable
> >> (--with-ld-errbuf-size), while yet leaving the default size of 128
> >> unaffected.
> > 
> > i dont think this is the best place to start adding configurations
> > 
> > i'd prefer simply increasing the buffer if this is an issue
> > 
> > c++ mangled names can get pretty insane, i just checked my system
> 
> check boost-spirit for the most amazing results (we are talking
> kilobytes for a single symbol)

In this case, I tend to think the current limit is just a bug. It
should suffice to just keep track of the current buffer size and
malloc a new buffer when a larger message is needed, leaving the old
message forever in the previous buffer. To avoid too many such
allocations, size can be doubled each time it's insufficient. If
allocation fails, the error message can be replaced with "internal
dynamic linker allocation failure" which will always fit in the old
buffer.

Note that in a correct program, the buffer should never be used
anyway, so it shouldn't lead to bloat.

Rich

Re: patch: make the size of errbuf configurable

[Z. Gilboa]

On 05/19/2013 05:03 PM, Rich Felker wrote:
> On Sun, May 19, 2013 at 04:12:58PM -0400, Z. Gilboa wrote:
>> Greetings,
>>
>> When a shared library that resides in a deeply nested folder
>> contains unresolved (long-named, mangled) symbols, the displayed
>> name of the library and/or symbol might get truncated.  The attached
>> patch makes the size of errbuf (ldso/dynlink.c) configurable
>> (--with-ld-errbuf-size), while yet leaving the default size of 128
>> unaffected.
> If at some point we do have build configurations, I'd want it to be
> more structured, and for the user-visible names not to reflect
> internal variable names, but something more meaningful to users.
>
> With that said, in this case I think a different solution is called
> for, mainly because having configurability for such small details is
> setting a precedent that, if followed, could lead to hundreds of
> trivial configurable knobs and the same testability nightmare that
> plagues uClibc.
>
> My preference is that either long pathnames should be truncated in a
> reasonable way (keep in mind that the message is *not* parsable by the
> caller; the only way it can be used is presenting it to the user)
certainly; the initial motivation was log-file processing.

> or
> larger buffers should be dynamically allocated. The only reason I did
> not go the dynamic-allocation path to begin with was to make it so
> non-thread-safe usage of dlerror yields (at worst) corrupted messages
> rather than crashes. This can also be achieved with dynamic allocation
> (as long as the old too-short buffer is never freed), but it's more
> complex.

In my understanding, the current approach of having a fixed buffer size 
is by far the superior one.  And I also see your point about keeping the 
number of configurable options to a minimum.   With this in mind, here 
are some alternatives:

1) remove the change from _configure_, but keep the change in 
_dynlink.c_ intact, which would allow developers to control the buffer 
size via CFLAGS.  Add a note to the documentation (and configure's 
--help) about the option to do so.

2) discard the patch altogether, but add a list of "strategic code 
locations" to the documentation...

Any thoughts?


>
> I'd welcome input on which approach users would prefer.
>
> Rich

Re: patch: make the size of errbuf configurable

[Rich Felker]

On Sun, May 19, 2013 at 05:51:05PM -0400, Z. Gilboa wrote:
> >My preference is that either long pathnames should be truncated in a
> >reasonable way (keep in mind that the message is *not* parsable by the
> >caller; the only way it can be used is presenting it to the user)
> certainly; the initial motivation was log-file processing.
> 
> >or
> >larger buffers should be dynamically allocated. The only reason I did
> >not go the dynamic-allocation path to begin with was to make it so
> >non-thread-safe usage of dlerror yields (at worst) corrupted messages
> >rather than crashes. This can also be achieved with dynamic allocation
> >(as long as the old too-short buffer is never freed), but it's more
> >complex.
> 
> In my understanding, the current approach of having a fixed buffer
> size is by far the superior one.

Could you elaborate as to why? Are you concerned about memory usage?
Code complexity? Or some other reason?

Rich

Re: patch: make the size of errbuf configurable

[Z. Gilboa]

On 05/19/2013 06:09 PM, Rich Felker wrote:
> On Sun, May 19, 2013 at 05:51:05PM -0400, Z. Gilboa wrote:
>>> My preference is that either long pathnames should be truncated in a
>>> reasonable way (keep in mind that the message is *not* parsable by the
>>> caller; the only way it can be used is presenting it to the user)
>> certainly; the initial motivation was log-file processing.
>>
>>> or
>>> larger buffers should be dynamically allocated. The only reason I did
>>> not go the dynamic-allocation path to begin with was to make it so
>>> non-thread-safe usage of dlerror yields (at worst) corrupted messages
>>> rather than crashes. This can also be achieved with dynamic allocation
>>> (as long as the old too-short buffer is never freed), but it's more
>>> complex.
>> In my understanding, the current approach of having a fixed buffer
>> size is by far the superior one.
> Could you elaborate as to why? Are you concerned about memory usage?
> Code complexity? Or some other reason?
A little bit of both, with complexity being the main factor.  As far as 
I can tell (from looking at dynlink.c and otherwise), there is only one 
case (do_relocs) where both the library name and symbol name are sent to 
the buffer.  So given the case's rarity and singularity, I would not 
introduce "complex" code or memory allocation into the function.  We 
should also remember that this is not about how the error is being 
handled, only about how it is being presented, meaning that less code is 
probably better...

>
> Rich

Re: patch: make the size of errbuf configurable

[Rich Felker]

On Sun, May 19, 2013 at 06:26:04PM -0400, Z. Gilboa wrote:
> >>In my understanding, the current approach of having a fixed buffer
> >>size is by far the superior one.
> >Could you elaborate as to why? Are you concerned about memory usage?
> >Code complexity? Or some other reason?
> A little bit of both, with complexity being the main factor.  As far
> as I can tell (from looking at dynlink.c and otherwise), there is
> only one case (do_relocs) where both the library name and symbol
> name are sent to the buffer.  So given the case's rarity and
> singularity, I would not introduce "complex" code or memory
> allocation into the function.  We should also remember that this is
> not about how the error is being handled, only about how it is being
> presented, meaning that less code is probably better...

From what I can see, complexity can be avoided and maybe even reduced
by refactoring the code so that all the places that set an error
message call a short simple function that wraps snprintf and allocates
a new buffer if needed. The complexity reduction would be if we can
eliminate duplicate logic at each call point, which I haven't checked
yet.

Rich

Re: patch: make the size of errbuf configurable

[Z. Gilboa]

On 05/19/2013 07:22 PM, Rich Felker wrote:
> On Sun, May 19, 2013 at 06:26:04PM -0400, Z. Gilboa wrote:
>>>> In my understanding, the current approach of having a fixed buffer
>>>> size is by far the superior one.
>>> Could you elaborate as to why? Are you concerned about memory usage?
>>> Code complexity? Or some other reason?
>> A little bit of both, with complexity being the main factor.  As far
>> as I can tell (from looking at dynlink.c and otherwise), there is
>> only one case (do_relocs) where both the library name and symbol
>> name are sent to the buffer.  So given the case's rarity and
>> singularity, I would not introduce "complex" code or memory
>> allocation into the function.  We should also remember that this is
>> not about how the error is being handled, only about how it is being
>> presented, meaning that less code is probably better...
>  From what I can see, complexity can be avoided and maybe even reduced
> by refactoring the code so that all the places that set an error
> message call a short simple function that wraps snprintf and allocates
> a new buffer if needed. The complexity reduction would be if we can
> eliminate duplicate logic at each call point, which I haven't checked
> yet.
>
> Rich

When moving beyond dynlink.c then yes, I believe, that should be 
beneficial.  I just had a quick look at the places where snprintf is 
used, and found that the following functions might benefit from the 
above wrapper:

dynlink.c:    all functions that call snprintf
syslog.c:    _vsyslog
getnameinfo
inet_ntop (unsure)
sem_open (unsure: _name_ can be up to 251 characters long 
(http://man7.org/linux/man-pages/man7/sem_overview.7.html), but is link 
to _tmp_ which is only up to 64 characters long)

Zvi

Re: patch: make the size of errbuf configurable

[Rich Felker]

On Sun, May 19, 2013 at 08:09:03PM -0400, Z. Gilboa wrote:
> > From what I can see, complexity can be avoided and maybe even reduced
> >by refactoring the code so that all the places that set an error
> >message call a short simple function that wraps snprintf and allocates
> >a new buffer if needed. The complexity reduction would be if we can
> >eliminate duplicate logic at each call point, which I haven't checked
> >yet.
> >
> >Rich
> 
> When moving beyond dynlink.c then yes, I believe, that should be
> beneficial.  I just had a quick look at the places where snprintf is
> used, and found that the following functions might benefit from the
> above wrapper:

I was just looking at dynlink.c, but we could consider whether the
same issue applies in other places. I doubt the same function would be
useful in other places though since some of the logic I'd want to
factor would be dynlink-specific. Basically, I would want the function
to also encapsulate the dynlink error handling code (usually longjmp
or printing an error message).

> dynlink.c:    all functions that call snprintf
> syslog.c:    _vsyslog

Indeed there's a question of what syslog should do when the message is
too long. But unboundedly-long messages can't really be supported
anyway; the ultimate upper limit is the max unix socket datagram size.

> getnameinfo
> inet_ntop (unsure)

Not necessary. All strings here are highly bounded in size, and in
most (all?) places they're using caller-provided buffers anyway.

> sem_open (unsure: _name_ can be up to 251 characters long
> (http://man7.org/linux/man-pages/man7/sem_overview.7.html), but is
> link to _tmp_ which is only up to 64 characters long)

I'm not sure what you're saying here. All of the strings here are
highly bounded in size, as you noted. There's certainly no need for
dynamic allocation of the name buffer, which would introduce an
additional failure case.

Rich

Re: patch: make the size of errbuf configurable

[Z. Gilboa]

On 05/19/2013 08:21 PM, Rich Felker wrote:
> On Sun, May 19, 2013 at 08:09:03PM -0400, Z. Gilboa wrote:
>>>  From what I can see, complexity can be avoided and maybe even reduced
>>> by refactoring the code so that all the places that set an error
>>> message call a short simple function that wraps snprintf and allocates
>>> a new buffer if needed. The complexity reduction would be if we can
>>> eliminate duplicate logic at each call point, which I haven't checked
>>> yet.
>>>
>>> Rich
>> When moving beyond dynlink.c then yes, I believe, that should be
>> beneficial.  I just had a quick look at the places where snprintf is
>> used, and found that the following functions might benefit from the
>> above wrapper:
> I was just looking at dynlink.c, but we could consider whether the
> same issue applies in other places. I doubt the same function would be
> useful in other places though since some of the logic I'd want to
> factor would be dynlink-specific. Basically, I would want the function
> to also encapsulate the dynlink error handling code (usually longjmp
> or printing an error message).
alles klar...
>
>> dynlink.c:    all functions that call snprintf
>> syslog.c:    _vsyslog
> Indeed there's a question of what syslog should do when the message is
> too long. But unboundedly-long messages can't really be supported
> anyway; the ultimate upper limit is the max unix socket datagram size.
>
>> getnameinfo
>> inet_ntop (unsure)
> Not necessary. All strings here are highly bounded in size, and in
> most (all?) places they're using caller-provided buffers anyway.
>
>> sem_open (unsure: _name_ can be up to 251 characters long
>> (http://man7.org/linux/man-pages/man7/sem_overview.7.html), but is
>> link to _tmp_ which is only up to 64 characters long)
> I'm not sure what you're saying here. All of the strings here are
> highly bounded in size, as you noted. There's certainly no need for
> dynamic allocation of the name buffer, which would introduce an
> additional failure case.
My mistake.  I wasn't sure why the size of _tmp_ was different, however 
I now see that the two sizes are independent of one another.

>
> Rich