1295056 - PL_strcmp returns inconsistent results when one of the arguments is NULL

Status: RESOLVED FIXED

(NSPR :: NSPR, defect)

Description

[Mike Hommey [:glandium]]

This was found because the string test in lib/tests/string.c fails on s390x, with:

Test 011 (PL_strcmp)      ...FAIL 1: (null)-word -> 2147460372, not -1

The code for PL_strcmp is currently:

    if( ((const char *)0 == a) || (const char *)0 == b ) 
        return (PRIntn)(a-b);
    return (PRIntn)strcmp(a, b);

So essentially, it's a wrapper around strcmp with NULL-ptr handling.

Now, what happens is that in that specific failure case, a is NULL, and b is a pointer to a static string "word". On most platforms, the pointer for "word" has a low address. However, on s390x, it has an address like 0x8000xxxx.

So a is 0, and b is 0x8000xxxx. s390x is 64-bits, so (a - b) is 0xffffffff7fffyyyy but it's cast to a PRIntn, which is an int, and that is truncated to 0x7fffyyyy (not sure why it doesn't preserve the sign).

So the returned value in that case is a large positive value instead of the expected negative value. And it doesn't really matter that there's a 64-bits to 32-bits conversion involved, with sign truncation, etc. It's clear there is a problem on even e.g. 32-bits x86, where the result for PL_strcmp(0, b) is opposite whether b is below or above 0x80000000.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Return consistent results for PL_str*cmp when one of the pointers is NULL

Comment 2

[Mike Hommey [:glandium]]

Attachment: Return consistent results for PL_str*cmp when one of the pointers is NULL

Turns out PL_str*casecmp is affected too.

Comment 3

[Kai Engert (:KaiE:)]

The code change looks correct to me, keeping the old intended behavior.

I don't yet understand why your tests will work.

Looking at the first test change:
          { (const char *)0, (const char *)-1, -1 },

It seems that ((const char *)-1) will be passed to PL_strcmp, which will pass it to strcmp.

Doesn't that mean strcmp will attempt to access the memory at this non-null address, and will likely crash? Or what am I missing?

Comment 4

[Kai Engert (:KaiE:)]

Apparently the NSPR tests aren't built. When trying to build them, we get immediate compiler errors (because of the recently introduced treat-warnings-as-errors). After trying to fix compiler errors in lib/tests/arena.c, I get build errors about incorrect path, so apparently the makefile rules for the tests directory aren't up to date either.

Comment 5

[Kai Engert (:KaiE:)]

Comment on attachment 8781279 [details] [diff] [review]
Return consistent results for PL_str*cmp when one of the pointers is NULL

If you want to check in the fix, without making the tests work:

r=kaie for all of your changes in directory lib/libc

Comment 6

[Kai Engert (:KaiE:)]

Attachment: testfix.patch

I used this quick hack to enable the tests on my system.

With this applied, I can execute the "string" test binary and I get success.

With your lib/libc fixes applied, I still get success.

With your test changes applied, I get failures.

Comment 7

[Wan-Teh Chang]

Comment on attachment 8781279 [details] [diff] [review]
Return consistent results for PL_str*cmp when one of the pointers is NULL

Review of attachment 8781279 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Thanks for the patch.

::: lib/libc/src/strcase.c
@@ +50,5 @@
>  
> +    if( (const char *)0 == a )
> +        return ((const char *)0 == b) ? 0 : -1;
> +    if( (const char *)0 == b )
> +        return 1;

Thanks for the fix. This is essentially the same as the
code at the beginning of SECITEM_CompareItem:

https://dxr.mozilla.org/nss/source/nss/lib/util/secitem.c#179

::: lib/tests/string.c
@@ +930,5 @@
>          }
>  
>          printf("FAIL %d: %s-%s -> %d, not %d\n", i,
> +               array[i].one ? (array[i].one == (const char *)-1 ? "(highptr)"
> +                                                                : array[i].one)

Nit: maybe this code should become a function.
There are eight copies of this code.

That function would also be a good place to document
the purpose of the (const char *)-1 pointer in tests.
Without a reference to this bug, it is not clear why
some test cases use the const char *)-1 pointer.
Note: I think it is not necessary to go this far in
testing.

Comment 8

[Wan-Teh Chang]

Comment on attachment 8782450 [details] [diff] [review]
testfix.patch

Review of attachment 8782450 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/tests/Makefile.in
@@ +71,5 @@
>  ifeq ($(OS_ARCH), Linux)
>      ifeq ($(OS_RELEASE), 1.2)
>          EXTRA_LIBS = -ldl
>      else
> +        LDOPTS += -Xlinker -rpath $(DESTDIR)$(bindir)

Kai: thank you for running these tests. The reason string.c
fails is that it is using the system NSPR libraries. The
change you made to this Makefile is incorrect. We need to
use a change similar to nspr/pr/tests/Makefile.in.
For a quick-and-dirty fix, just change
    
    $(PWD)/$(dist_libdir)

to

    $(dist_libdir)

Comment 9

[Kai Engert (:KaiE:)]

No, I tried to use $(dist_libdir), but that doesn't have the correct path.

I used

Comment 10

[Kai Engert (:KaiE:)]

Please ignore my previous comments. In my quick experiments I didn't notice that nspr wasn't rebuilt for each of my attempted makefile changes.

And now I see why the suggested test change is safe, because you're only using (char*)-1 in combination with the second parameter (char*)0, so the tests never reach the underlying comparison calls.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Return consistent results for PL_str*cmp when one of the pointers is NULL

Changed the test parts.

Comment 12

[Mike Hommey [:glandium]]

Comment on attachment 8782652 [details] [diff] [review]
Return consistent results for PL_str*cmp when one of the pointers is NULL

Review of attachment 8782652 [details] [diff] [review]:
-----------------------------------------------------------------

Meh, I agree it's not necessary to go this far in testing. I'll just remove the test part. They were useful to show the problem on the existing code without building on s390x, and was useful to verify the fix works, but it's not going to achieve much on the long run.

Comment 13

[Mike Hommey [:glandium]]

https://hg.mozilla.org/projects/nspr/rev/6cf50e545775