Hacker News Reader

Comment by SketchySeaBeast | Hacker News Reader

As someone who doesn't understand the language's nuances - what does adding a cast to a value being assigned in the function open? Will it return NULL more often?

View on HN

Sign confusion: malloc takes size_t, so negative numbers are just really large numbers to malloc. A negative int passes the check after adding the cast, because we’re now comparing with signs and now a negative number is smaller than a positive one, so malloc will be called with the larger value. Since it gets multiplied you will also overflow and get pretty much any size. There’s a DoS opportunity at least.

The correct fix is to cast the input to unsigned, I think.

Ouf, I wish C had better type checking. I'm working in PHP and this would've been caught in strict mode there - why wouldn't malloc raise an objection over being passed a signed value?

If you run it with `-Wsign-conversion`, it will catch this.

https://stackoverflow.com/questions/27490762/how-can-i-conve...

Because malloc does not throw exceptions (in C++), in C there is no concept of exceptions at all, so there is no objection being raised. The developer is treated as responsible for verifying the memory and overflows themselves. The only thing you are promised is that if the malloc operation itself fails you will get a result of NULL;

munk-a’s question was more along the lines of “why would the language allow a signed int to be silently converted to an unsigned int” than “what should malloc return for negative input”.

Oh, perhaps I misunderstood the intent then.

malloc has no idea what negative input is, because it takes an (unsigned) size_t. Negative numbers just end up being large positive numbers (many allocators will check for this, though).

The compiler is aware that a function taking an unsigned size_t is being given a signed value though - it could mandatorily throw up a warning (and apparently there is a switch for this) which, if it were on, actually protects against the issue raised in this specific security vulnerability.

Technically, malloc doesn't get passed a signed value. malloc takes a size_t (which is unsigned). The conversion happens at the call site.

The compiler (or linker I guess) should have visibility of this conversion. I guess to rephrase, why is this conversion being necessary not a compile warning by default, you are changing the range the type can store possibly leading to truncation or misinterpretation.

That would be the compiler, not the linker. Regarding the warnings, compilers must strike a balance between positives and false positives (just that a number is of type int need not necessarily mean that it is negative). With most compilers you can select a warning level or even enable or disable specific warnings, like "sign conversions".

C implicitly changes among numeric types without requiring any conversion operator.

Parameter passing works much like assignment. Just like you can assign an int value to an unsigned int variable, you can pass an int argument to an unsigned int parameter.

There is free conversion among char, int, long, and the floating-point types. For instance int x = 1.0 is valid.

Many of these converisons have implementation-defined behavior if the value doesn't fit into the range of the destination type. Assigning a long long to a char where the value does not fit, for instance.

In an integer-floating conversion or vice versa, if the value does not fit, the behavior is undefined. No diagnostic required. E.g. the program can die with a "floating point exception", or just putter along with a random incorrect value.

The conversion rules themselves are implementation-specific because the sizes and ranges of the type vary, as well as certain details like whether plain char is signed or not.

For instance, if int and long have the same range (e.g. both are 32 bits wide), then converting any value between them is well-defined. If they don't (e.g. 16 bit int, 32 bit long), then converting long to int can truncate.

Some things can be counted upon: signed char will convert to short without loss; short to int, int to long, long to long long. Also float to double.

That doesn't make sense -- how would a negative value of num be greater than the positive value that is (int)limit? It must be something else...

Edit: Oh, I see what I missed -- I was thinking of the danger as directly passing too large a number to malloc, when actually the danger is passing a negative number, which then gets implicitly converged to a large positive number. Oops.

  #include <stdio.h>
  
  int main(void)
  {
    unsigned int plus_one = 1;
    int minus_one = -1;
  
    if(plus_one < minus_one)
      printf("1 < -1");
    else
      printf("boring");
  
    return 0;
  }

Gotten from S/O.

This is an example of code that would trigger what the OP describes as a "harmless warning"

The problem arises because doing a signed comparison is just fine, but when you call malloc it gets converted to an unsigned integer and allocated.

Meaning -1, which passes the signed check fine, becomes 0xFFFFFFFF as a 32 bit unsigned integer...

Now, theoretically a well configured compiler that complains about one would also complain about the other, catching the signed value going to malloc as a warning.

Because C's implicit integer promotion rules are horrible. If you are operating on two signed types, or two unsigned types, it does what you would expect and converts both to the type with greater rank (essentially more bits). Likewise, if you have one signed type and one unsigned type and the signed type has greater rank then it does the sensible thing and converts them both to the signed type.

However, if you have one signed type and one unsigned type, and the unsigned type has greater or equal rank, then it converts to the unsigned type, despite the fact that the unsigned type can't represent any of the negative values of the signed type, so they get wrapped!

This is the cause of so many bugs.

But the irony is that the [immediate] bug in this case was adding an explicit cast, which is also exactly what people would have done in languages that didn't implicitly promote to unsigned.

The cleverness of this implicit backdoor hypothetical is that it would have worked substantially the same way in most other low-level languages, including Rust. Modern C compilers literally complain in the same way as stricter languages, which had the effect of inducing a poorly considered cast.

Yeah, it frustrates me that both Go and Rust have decided to require casting for all numeric conversions, whether there is a chance of information loss or not. This causes casting to proliferate, and you grow numb to it. If lossless conversions were implicit, then you would only need casts in potentially lossy situations so they would stand out more, and hopefully make you think harder about correctness.

The multiplication can end up turning that large negative number into a small positive one, likely giving you a chance to smash the heap later on (depending on exactly how the int is used).

Exactly right. Here's a runnable sample! https://repl.it/repls/ShinyQueasyLocks

> The correct fix is to cast the input to unsigned, I think.

I think the correct fix is rather to check the input (e.g. assert()). What you suggest means reinterpreting what the caller wanted, and that leads to nasty surprises down the road.

The correct fix is to change the type of the argument to allocatebufs to size_t. Casts are a code smell, and often the best way to fix these warnings is to work your way backward through the code fixing up the types to something more appropriate. Ideally you'd reach the point where someone could induce a negative value, but in any event simply changing the argument type would have sufficed to safeguard the limit check.

That said, the latest release of glibc would have failed on a negative expression promoted to unsigned:

> Memory allocation functions malloc, calloc, realloc, reallocarray, valloc, pvalloc, memalign, and posix_memalign fail now with total object size larger than PTRDIFF_MAX. This is to avoid potential undefined behavior with pointer subtraction within the allocated object, where results might overflow the ptrdiff_t type.

https://sourceware.org/ml/libc-alpha/2019-08/msg00029.html

The new glibc behavior doesn't help with the multiplication overflow, though, as it could have wrapped to a positive value (notwithstanding the fact that such wrapping is undefined). Which is why if I saw code like this I would've worked my way back to all the call sites to understand why the argument to allocatebufs wasn't size_t to begin with. Maybe they were C++ programmers--for decades "best practice" in C++ was to use int for the size of objects.

Arithmetic overflow leading to potential buffer overflow is still a problem even in languages like Rust. Nobody should ever add an integer cast somewhere without having done their due diligence. Which is why stricter arithmetic type checking can sometimes be a double-edged sword--too many people will add an explicit cast and move on.

Fair, I only meant to fix the warning, not improve the code quality. I think the problem here is probably something more along the lines of a bad abstraction, as I get the feeling the idea is "I have an int, I need to safely allocate this much memory within limits." If you just change the interface to size_t, that would push the problem down further, but at some point I'd ask why bother with this helper function.

Iirc size_t is unsigned and int is signed, normally, but raw casting in c compares bits, so limit will always be very large. Take with a grain of of salt this info, I can't verify right now.

Edit: proper explanation above. I got some things wrong.

Reply on news.ycombinator.com