4 ms·

An Experiment with Checking the glibc Library

19 points by AndreyKarpov 13y ago

fexl 13y ago
Looks like yet another dependency that I'll have to build from source code, and test, fix, and refactor myself.
al2o3cr 13y ago
I'm not buying these conclusions - on a quick scan, the "strange expression" isn't wrong: the code is scanning to find the first character that isn't a colon. The check against the zero byte is harmless, and I could imagine other static analysis tools insisting on it (as cheap insurance against scanning off-the-end). As for the "shifting negative integers is undefined behavior", this is a technically-true statement that's been practically-false for quite a few years - does anybody still use a machine that doesn't use two's-complement integers?
- JoachimSchipper 13y ago
  The strange expression does seem wrong to me: the loop should terminate on end-of-line or ":", so continue while not end-of-line and not ":". I think you're right that shifting negative integers is fine on most any platform you're likely to encounter, but until recently one could also assume that adding two signed integers would wrap in practice, despite being undefined in theory; due to gcc optimizing ever more aggressively, code assuming that signed integers wrap is now broken (see e.g. http://thiemonagel.de/2010/01/signed-integer-overflow/ http://thiemonagel.de/2010/01/signed-integer-overflow/). So I'd be careful with shifts of negative integers in my own code.
  - jibsen 13y ago
    It's skipping over ':' characters (which serve as delimiters) to get to the next token of the string (or end of line). So it looks correct, but the check for end of line is superfluous (unless any architecture uses ':' as end of line marker of course). I think it is more likely it looks the way it does because it was made by copy-pasting and editing the loop a few lines further down that copies a token: while (categoryvalue[0] != '\0' && categoryvalue[0] != ':') *cp++ = *categoryvalue++; As for the shifting, I would assume glibc is free to expect whatever holds true on the platforms it currently supports. But fair enough warning about it in a analysis tool.
nkurz 13y ago
The compiler is allowed to remove the call of the memset() function when compiling the Release version. Well, it is actually obliged to do so for the sake of optimization. Since the 'temp_result' buffer is not used anywhere after calling the memset() function, the function call itself is not needed too. I'm constantly amazed by the perversity of 'compliant' compilers, but can this be true? It seems really unlikely unless memset() is treated specially in the spec. Presumably a compiler is not 'obliged' to remove calls to write() or free() just because the argument is not used again after the call. How can it tell the difference? I've always presumed that if I call a function in a shared library, that the call will be made. Is this not the case?
- unwind 13y ago
  Well, I would guess that since memset() is a well-known standard library function that actually has a specification, and its buffer argument is not volatile, then the compiler can "get away" with assuming things. It's the same for e.g. strcmp() and strcpy(), they can be compiled into specialized instructions when the compiler considers that a win. GCC's set of builtins is quite large: http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html. To comment on the (very interesting) article, I think this: XDR *xdrs = &clp->xdr_stream; is not at all an error, even if clp is NULL. No de-referencing occurs, since the expression is just the argument of the & (address-of, in C) operator. See the traditional offsetof() definition: http://en.wikipedia.org/wiki/Offsetof http://en.wikipedia.org/wiki/Offsetof which explicitly does this for a base pointer of 0.
- stusmall 13y ago
  The reason why this is allowed is that is seemingly pointless code. After this buffer is memset'd it is never referenced again and falls out of scope. Assuming that all other code behaves safe and doesn't rely on undefined behavior it is unreachable. It doesn't matter if you set it to zero or not if no one reads it. For a lot of code this is a valid assumption and a safe optimizations. Read and write calls are special since they do have side effects that can't be optimized out by a compiler. Someone please correct me if I am misunderstanding this code snippet. I am fine if a compiler makes an optimization like this for my code but I can understand why this is unacceptable for crypto.
stusmall 13y ago
>One might expect the glibc library's code to be written in such a way as to provide maximum portability. Its worth noting portability was one of the reasons cited by Debian for moving from glibc to eglibc.
acqq 13y ago
I am shocked to see the strlen expansion. Is it common or something specific for some very special compiler options? I can't imagine a lot of cases where I would want that in my code.

19 points by AndreyKarpov 13y ago

fexl 13y ago

Looks like yet another dependency that I'll have to build from source code, and test, fix, and refactor myself.

I'm not buying these conclusions - on a quick scan, the "strange expression" isn't wrong: the code is scanning to find the first character that isn't a colon. The check against the zero byte is harmless, and I could imagine other static analysis tools insisting on it (as cheap insurance against scanning off-the-end). As for the "shifting negative integers is undefined behavior", this is a technically-true statement that's been practically-false for quite a few years - does anybody still use a machine that doesn't use two's-complement integers?

JoachimSchipper 13y ago
The strange expression does seem wrong to me: the loop should terminate on end-of-line or ":", so continue while not end-of-line and not ":". I think you're right that shifting negative integers is fine on most any platform you're likely to encounter, but until recently one could also assume that adding two signed integers would wrap in practice, despite being undefined in theory; due to gcc optimizing ever more aggressively, code assuming that signed integers wrap is now broken (see e.g. http://thiemonagel.de/2010/01/signed-integer-overflow/ http://thiemonagel.de/2010/01/signed-integer-overflow/). So I'd be careful with shifts of negative integers in my own code.
- jibsen 13y ago
  It's skipping over ':' characters (which serve as delimiters) to get to the next token of the string (or end of line). So it looks correct, but the check for end of line is superfluous (unless any architecture uses ':' as end of line marker of course). I think it is more likely it looks the way it does because it was made by copy-pasting and editing the loop a few lines further down that copies a token: while (categoryvalue[0] != '\0' && categoryvalue[0] != ':') *cp++ = *categoryvalue++; As for the shifting, I would assume glibc is free to expect whatever holds true on the platforms it currently supports. But fair enough warning about it in a analysis tool.

nkurz 13y ago

The compiler is allowed to remove the call of the memset() function when compiling the Release version. Well, it is actually obliged to do so for the sake of optimization. Since the 'temp_result' buffer is not used anywhere after calling the memset() function, the function call itself is not needed too. I'm constantly amazed by the perversity of 'compliant' compilers, but can this be true? It seems really unlikely unless memset() is treated specially in the spec. Presumably a compiler is not 'obliged' to remove calls to write() or free() just because the argument is not used again after the call. How can it tell the difference? I've always presumed that if I call a function in a shared library, that the call will be made. Is this not the case?

unwind 13y ago
Well, I would guess that since memset() is a well-known standard library function that actually has a specification, and its buffer argument is not volatile, then the compiler can "get away" with assuming things. It's the same for e.g. strcmp() and strcpy(), they can be compiled into specialized instructions when the compiler considers that a win. GCC's set of builtins is quite large: http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html http://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html. To comment on the (very interesting) article, I think this: XDR *xdrs = &clp->xdr_stream; is not at all an error, even if clp is NULL. No de-referencing occurs, since the expression is just the argument of the & (address-of, in C) operator. See the traditional offsetof() definition: http://en.wikipedia.org/wiki/Offsetof http://en.wikipedia.org/wiki/Offsetof which explicitly does this for a base pointer of 0.
stusmall 13y ago
The reason why this is allowed is that is seemingly pointless code. After this buffer is memset'd it is never referenced again and falls out of scope. Assuming that all other code behaves safe and doesn't rely on undefined behavior it is unreachable. It doesn't matter if you set it to zero or not if no one reads it. For a lot of code this is a valid assumption and a safe optimizations. Read and write calls are special since they do have side effects that can't be optimized out by a compiler. Someone please correct me if I am misunderstanding this code snippet. I am fine if a compiler makes an optimization like this for my code but I can understand why this is unacceptable for crypto.

stusmall 13y ago

>One might expect the glibc library's code to be written in such a way as to provide maximum portability. Its worth noting portability was one of the reasons cited by Debian for moving from glibc to eglibc.

acqq 13y ago

I am shocked to see the strlen expansion. Is it common or something specific for some very special compiler options? I can't imagine a lot of cases where I would want that in my code.