musl's putenv makes assumptions about memcmp

musl's putenv makes assumptions about memcmp

[Pascal Cuoq]

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

Hello,

Quoting myself from a previous report:

when I started testing parts of musl with TIS Interpreter, I made sure to use TIS Interpreter versions of low-level functions such as memcpy and memset, while testing higher-level functions. Musl's functions can provide guarantees beyond the standard, and it is fair game to rely on these guarantees elsewhere in musl since musl's versions of these functions are called, but I thought it would be interesting to know that musl provides additional guarantees and relies on them.

An interesting new example just turned up.

According to the succinct description of memcmp in the C standard, and to the interpretation of Glibc's developers, memcmp should only be called with fully valid buffers, even when a sequential comparison of the two buffers would not make any out-of-bounds access. Please see https://trust-in-soft.com/memcmp-requires-pointers-to-fully-valid-buffers/ for details.

musl's implementation of putenv uses memcmp exactly like the blog post says it shouldn't:

https://git.musl-libc.org/cgit/musl/tree/src/env/putenv.c?id=80bf5952551c002cf12d96deb145629765272db0

This doesn't matter now, because the implementation of memcmp used is musl's, which does sequentially compare characters and does stop as early as it knows what to return: https://git.musl-libc.org/cgit/musl/tree/src/string/memcmp.c?id=80bf5952551c002cf12d96deb145629765272db0

However this behavior of musl's memcmp should be documented and preserved in future versions. Otherwise introducing a memcmp optimization in musl similar to Glibc's would silently break putenv, making it segfault in rare and perhaps hard to reproduce circumstances.

Alternately, it is possible to make putenv rely only on the standard behavior of the functions it calls. It seems to me that strncmp can be used here. The difference in execution speed between memcmp and strncmp should not matter in this context.

Pascal


[-- Attachment #2: Type: text/html, Size: 3166 bytes --]

Re: musl's putenv makes assumptions about memcmp

[Alexander Monakov]

*env functions have multiple issues including other UB and a memory leak.

http://openwall.com/lists/musl/2016/03/13/7

Alexander

Re: musl's putenv makes assumptions about memcmp

[Pascal Cuoq]

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

Hello Alexander,

Thanks for this pointer. This UB and a similar one in putenv were going to be our next report:

unsetenv:
* rewrite; this fixes UB caused by testing a free'd pointer against
  NULL on entry to subsequent loops.

In the version after your patch, we do not detect any undefined behavior with TIS Interpreter, for the tests we have (libc-testsuite plus one additional test that we wrote to make TIS Interpreter confirm the misuse of memcmp in putenv).

Pascal

On 21 Aug 2017, at 10:02, Alexander Monakov <amonakov@ispras.ru<mailto:amonakov@ispras.ru>> wrote:

*env functions have multiple issues including other UB and a memory leak.

http://openwall.com/lists/musl/2016/03/13/7

Alexander


[-- Attachment #2: Type: text/html, Size: 1571 bytes --]