Bug 11120 - strncmp() will be broken if used in ld.so on x86-64
Summary: strncmp() will be broken if used in ld.so on x86-64
Status: RESOLVED FIXED
Alias: None
Product: glibc
Classification: Unclassified
Component: libc (show other bugs)
Version: 2.11
: P2 normal
Target Milestone: ---
Assignee: Dmitry V. Levin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-12-31 11:00 UTC by Mark Seaborn
Modified: 2014-06-30 20:30 UTC (History)
4 users (show)

See Also:
Host:
Target:
Build:
Last reconfirmed:
fweimer: security-


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Seaborn 2009-12-31 11:00:49 UTC
As of 7956a3d2, sysdeps/x86_64/strcmp.S implements optimised versions
of both strcmp() and strncmp().  When this file is built into ld.so,
however, it falls back to using the unoptimised code which implements
only strcmp().  If strncmp() is ever used in ld.so on x86-64 it will
silently be implemented as strcmp().

Whether to fix this is upto the glibc developers.  If you don't want
to fix this I would suggest adding the following to the top of
sysdeps/x86_64/strncmp.S so that strncmp()'s behaviour does not come
as a surprise if it is ever used in this context:

#ifdef NOT_IN_libc
#  error "Not implemented for ld.so"
#endif
Comment 1 Ulrich Drepper 2010-01-14 16:10:14 UTC
Changed in git.
Comment 2 Don Hatch 2013-03-21 06:42:25 UTC
For reference, the change was:

commit f69190e74a081f0a35906ff0b9a8dc24e42341e8
Author: Ulrich Drepper <drepper@redhat.com>
Date:   Thu Jan 14 08:09:32 2010 -0800

    Prevent silent errors should x86-64 strncmp be needed outside libc.
Comment 3 Dmitry V. Levin 2013-03-21 23:42:04 UTC
Due to a typo repeated several times, this bug hasn't been fixed yet,
despite being closed for more than two years.
Comment 4 Dmitry V. Levin 2013-03-21 23:42:42 UTC
I'll submit a patch shortly.
Comment 5 Don Hatch 2013-03-22 02:48:03 UTC
Interesting!
So what was Ulrich's intent in that commit--
to fix the function,
or to make it abort if called from inside ld.so?
I couldn't tell from the commit message
or bug closure comment.

The reason I started looking at this bug
is I was noticing flakiness of getenv() when called inside ld.so;
then when I tried to implement my own my_getenv as a workaround,
I ran into the fact that strncmp() is flaky too
(which is why getenv is flaky-- it uses strncmp),
and I had to implement a my_strncmp as well.

This is really hard to nail down--
sometimes strncmp("abc", "ab", 2) returns 0, and sometimes it returns 1.
It seems consistent within a given compile,
but other than that I don't see much rhyme or reason to it.

I can reproduce the inconsistency by sticking the following
at the top of the _dl_sort_fini function body in elf/dl-fini.c:

    #define PRINT_STRNCMP(a,b,len) _dl_debug_printf("strncmp(\"%s\", \"%s\", %u) returned %u\n", (a), (b), (int)(len), (int)strncmp((a), (b), (len)));
    PRINT_STRNCMP("abc", "ab", 2); // prints correct result 0
    char abc[] = {'a','b','c','\0'};
    char ab[] = {'a','b','\0'};
    PRINT_STRNCMP(abc, ab, 2);     // prints incorrect result 1

It prints something like:
     30747:     strncmp("abc", "ab", 2) = 0
     30747:     strncmp("abc", "ab", 2) = 1

I was about to open a new bug report,
but at this point I'm not sure whether that would be a dup of this one?
Comment 6 Dmitry V. Levin 2013-03-22 03:30:49 UTC
(In reply to comment #5)
> Interesting!
> So what was Ulrich's intent in that commit--
> to fix the function,
> or to make it abort if called from inside ld.so?
> I couldn't tell from the commit message
> or bug closure comment.

The commit message says:
"Prevent silent errors should x86-64 strncmp be needed outside libc"
The intent clearly was to make build fail.

The bug is fixed finally:
http://sourceware.org/git/?p=glibc.git;a=commitdiff;h=glibc-2.17-458-g2e0fb52
Comment 7 Dmitry V. Levin 2013-03-22 03:37:26 UTC
(In reply to comment #5)
> The reason I started looking at this bug
> is I was noticing flakiness of getenv() when called inside ld.so;

getenv() is not normally called inside rtld.
Are you talking about some patched rtld?
Comment 8 Don Hatch 2013-03-22 04:03:34 UTC
> The commit message says:
> "Prevent silent errors should x86-64 strncmp be needed outside libc"
> The intent clearly was to make build fail.

That commit message reads as completely ambiguous to me,
so thank you for the clarification.
Comment 9 Don Hatch 2013-03-22 04:30:08 UTC
(In reply to comment #7)
> (In reply to comment #5)
> > The reason I started looking at this bug
> > is I was noticing flakiness of getenv() when called inside ld.so;
> 
> getenv() is not normally called inside rtld.
> Are you talking about some patched rtld?

I'm using it for debugging.
(There are also uses of getenv and strncmp throughout the elf subdirectory
which I mistakenly thought were part of rtld... apparently not).

Thanks for the fix.  It will prevent a lot of further wasted time.

Can you comment on the fact that, prior to your latest change
which now prevents calling it altogether,
strncmp was returning inconsistent results?
This is contrary to what I would expect from Mark Seaborn's original
description "If strncmp() is ever used in ld.so on x86-64 it will
silently be implemented as strcmp()".
Comment 10 Dmitry V. Levin 2013-03-22 12:00:54 UTC
(In reply to comment #9)
> Can you comment on the fact that, prior to your latest change
> which now prevents calling it altogether,
> strncmp was returning inconsistent results?
> This is contrary to what I would expect from Mark Seaborn's original
> description "If strncmp() is ever used in ld.so on x86-64 it will
> silently be implemented as strcmp()".

There is no code in rtld that directly or indirectly calls strncmp.
The change I committed is fixed safeguards that in case rtld is changed to use strncmp, x86-64 build would fail until strncmp is implemented for rtld.

It's certainly better to let rtld build fail rather than allow strncmp to be implemented as strcmp.
Comment 11 Don Hatch 2013-03-22 22:21:35 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > Can you comment on the fact that, prior to your latest change
> > which now prevents calling it altogether,
> > strncmp was returning inconsistent results?
> > This is contrary to what I would expect from Mark Seaborn's original
> > description "If strncmp() is ever used in ld.so on x86-64 it will
> > silently be implemented as strcmp()".
> 
> There is no code in rtld that directly or indirectly calls strncmp.
> The change I committed is fixed safeguards that in case rtld is changed to use
> strncmp, x86-64 build would fail until strncmp is implemented for rtld.
> 
> It's certainly better to let rtld build fail rather than allow strncmp to be
> implemented as strcmp.

Hi Dmitry,

I understand, and I completely agree with you,
as I've tried to say already.
There is absolutely no disagreement or question there.
After your latest change, I observe that any attempt to call strncmp directly,
or indirectly (e.g. by attempting to call getenv), fails at compile time.
That's a very good thing.  Thank you.

However, you have not answered my question.

My question is about the fact that,
contrary to what both you and Mark Seaborn have said,
prior to your change,
strncmp was *not* acting like strcmp, at least not consistently.
On one call it acted like strcmp, on the next call
(with exactly the same effective input) it didn't.
For a simple example of this,
please look carefully at my test case above (in my note of 2013-03-22 02:48:03).

Is this a surprise or not?
My concern is that your disabling of the function inside rtld
may be pushing more mistakes or rottenness under the rug.
Comment 12 Dmitry V. Levin 2013-03-22 23:00:44 UTC
(In reply to comment #11)
> There is absolutely no disagreement or question there.
> After your latest change, I observe that any attempt to call strncmp directly,
> or indirectly (e.g. by attempting to call getenv), fails at compile time.
> That's a very good thing.  Thank you.
> 
> However, you have not answered my question.
> 
> My question is about the fact that,
> contrary to what both you and Mark Seaborn have said,
> prior to your change,
> strncmp was *not* acting like strcmp, at least not consistently.
> On one call it acted like strcmp, on the next call
> (with exactly the same effective input) it didn't.

If you have a look at the implementation, you'll see that it's all the same in the non-libc case no matter by what name it is called.
The reason why you can see different results is that gcc is capable to optimize out some strncmp calls.  For example, in your case

$ echo -e '#include <string.h>\nint foo(void){return strncmp("abc", "ab", 2);}' |gcc -O2 -E - |tail -1
int foo(void){return (__extension__ (__builtin_constant_p (2) && ((__builtin_constant_p ("abc") && strlen ("abc") < ((size_t) (2))) || (__builtin_constant_p ("ab") && strlen ("ab") < ((size_t) (2)))) ? __extension__ ({ size_t __s1_len, __s2_len; (__builtin_constant_p ("abc") && __builtin_constant_p ("ab") && (__s1_len = strlen ("abc"), __s2_len = strlen ("ab"), (!((size_t)(const void *)(("abc") + 1) - (size_t)(const void *)("abc") == 1) || __s1_len >= 4) && (!((size_t)(const void *)(("ab") + 1) - (size_t)(const void *)("ab") == 1) || __s2_len >= 4)) ? __builtin_strcmp ("abc", "ab") : (__builtin_constant_p ("abc") && ((size_t)(const void *)(("abc") + 1) - (size_t)(const void *)("abc") == 1) && (__s1_len = strlen ("abc"), __s1_len < 4) ? (__builtin_constant_p ("ab") && ((size_t)(const void *)(("ab") + 1) - (size_t)(const void *)("ab") == 1) ? __builtin_strcmp ("abc", "ab") : (__extension__ ({ const unsigned char *__s2 = (const unsigned char *) (const char *) ("ab"); register int __result = (((const unsigned char *) (const char *) ("abc"))[0] - __s2[0]); if (__s1_len > 0 && __result == 0) { __result = (((const unsigned char *) (const char *) ("abc"))[1] - __s2[1]); if (__s1_len > 1 && __result == 0) { __result = (((const unsigned char *) (const char *) ("abc"))[2] - __s2[2]); if (__s1_len > 2 && __result == 0) __result = (((const unsigned char *) (const char *) ("abc"))[3] - __s2[3]); } } __result; }))) : (__builtin_constant_p ("ab") && ((size_t)(const void *)(("ab") + 1) - (size_t)(const void *)("ab") == 1) && (__s2_len = strlen ("ab"), __s2_len < 4) ? (__builtin_constant_p ("abc") && ((size_t)(const void *)(("abc") + 1) - (size_t)(const void *)("abc") == 1) ? __builtin_strcmp ("abc", "ab") : (__extension__ ({ const unsigned char *__s1 = (const unsigned char *) (const char *) ("abc"); register int __result = __s1[0] - ((const unsigned char *) (const char *) ("ab"))[0]; if (__s2_len > 0 && __result == 0) { __result = (__s1[1] - ((const unsigned char *) (const char *) ("ab"))[1]); if (__s2_len > 1 && __result == 0) { __result = (__s1[2] - ((const unsigned char *) (const char *) ("ab"))[2]); if (__s2_len > 2 && __result == 0) __result = (__s1[3] - ((const unsigned char *) (const char *) ("ab"))[3]); } } __result; }))) : __builtin_strcmp ("abc", "ab")))); }) : strncmp ("abc", "ab", 2)));}
Comment 13 Don Hatch 2013-03-26 05:43:37 UTC
(In reply to comment #12)
> (In reply to comment #11)
> > 
> > My question is about the fact that,
> > contrary to what both you and Mark Seaborn have said,
> > prior to your change,
> > strncmp was *not* acting like strcmp, at least not consistently.
> > On one call it acted like strcmp, on the next call
> > (with exactly the same effective input) it didn't.
> 
> If you have a look at the implementation, you'll see that it's all the same in
> the non-libc case no matter by what name it is called.
> The reason why you can see different results is that gcc is capable to optimize
> out some strncmp calls.  For example, in your case
> 
> $ echo -e '#include <string.h>\nint foo(void){return strncmp("abc", "ab", 2);}'
> |gcc -O2 -E - |tail -1

(in tcsh, this needs to say /bin/echo so it will understand the -e flag)

Ah!  Thank you, that makes perfect sense to me now.
It must be that __builtin_constant_p (abc) is returning false
but __builtin_constant_p("abc") is returning true,
resulting in two different execution paths taken, one of which actually
calls the function and the other doesn't.

Thanks very much for taking the time to examine and explain this.
Comment 14 Federico Sacerdoti 2013-06-12 03:03:17 UTC
Glad to find this, saved quite a bit of head-scratching. Below is my implementation of getenv that doesnt use strncmp. 

<pre>
static
char* getenv2(char **envp, const char* name)
{
  char **ep;
  size_t namelen = strlen(name);
  int i=0;
  for (ep = envp; *ep != NULL; ++ep)
  {
    for (i=0; (*ep)[i] == name[i] && i<namelen; i++)
      ;
    if (i == namelen && (*ep)[namelen] == '=')
      return &(*ep)[namelen+1];
  }
  return NULL;
}
</pre>