32 comments

[ 2.5 ms ] story [ 76.8 ms ] thread
This is why I never ever use size_t over int. I’m a newbie C++ programmer tho, wrote less than 10kloc of C++ code, but my both feet are bleeding cause of exactly this.
Be careful, int is usually 32-bit even on 64-bit systems. Using int all over is a good way to experience mysterious failures when dealing with chunks of data larger than 2GB, which is definitely a real possibility these days. Consider ssize_t, intptr_t, or int64_t any time you’re not absolutely sure you’ll never exceed 2^31-1.
Int is only 32 bits on 64-bit Windows, that for some reason didn't go along with the times. Every other mainstream 64 bits environment has 64 bits ints, even on ARM.

But it's only guaranteed to be 16 bits anyway, the same guarantee you have for size_t.

I think the point being made is that size_t is guaranteed to be large enough to hold the size of any object. The other integer types don't have this guarantee.
Size_t is guaranteed to hold values up to 65535. That's twice as long as int and much smaller than long.
size_t is guaranteed to hold values large enough to index into any array you can allocate. On a 64-bit system with an allocator that supports allocations larger than 4GB, that means size_t has to be larger than 32 bits. If it's an LLP64 system, size_t will be larger than long. If the value represents an index or size of something in memory, use size_t. Consider ssize_t if you need negative values and are certain you never need the larger half of the size_t range.
That's long, not int. Approximately nobody has 64 bit int.
Huh? Linux and Darwin both have 32-bit int. I’m sure there are some where it’s 64-bit, but the only one I’m aware of is old Cray systems.

C makes it hard to have anything bigger than 32-bit int. The standard integer types are char, short, int, long, and long long, and if int is 64-but then you’ll lack a standard type for 8, 16, or 32 bit integers, which is inconvenient. Nothing says you have to have those, or that you can’t make some implementation-specific type to fill the gap, but it’s messy.

With C99 I think it's better to use the fixed (or minimum) sized types from stdint.h when my requirements differ from what C guarantees. I prefer to be as explicit as possible in code (when appropriate) so as to to avoid this sort of confusion.
I generally agree. I'm speaking more from the implementor's side: if you do ILP64, then you either won't have one of 8, 16, or 32-bit integers, or you'll have to make some non-standard type to cover the gap. LP64 and LLP64 let you cover all the sizes you typically want with the standard types. That is, I assume, why 32-bit int on 64-bit systems is so common.
The split is LP64 (long is 64 bits, typically Unix) and LLP64 (long long is 64 bits, Windows).
Overflow and underflow on signed types is undefined behavior in C.

You should almost always use unsigned types unless you must use a signed type. And you need to think about these kinds of overflow and underflow cases.

Standard library operations such as length() on a string are defined to return to return a size_type, which is usually going to be an unsigned integer (https://en.cppreference.com/w/cpp/string/basic_string). So, you don't always have the option of avoiding it. You could convert to a signed integer, but that's usually going to be a narrowing conversion, which will introduce different subtle behavior. Unfortunately, in C and C++, there's no silver bullet for avoiding subtleties with integers.
I feel like doing arithmetic with unsigned integers is like doing all your coding 2 feet from a cliff, in return for more land about 2^63 miles away. It's fine as long as you never make the tinyest step wrong, and its very unlikely you'll ever need that other land (and it's usually easy to design code not to need it).

This doesn't apply if you are using unsized for bit twiddling, but then you shouldn't be using minus anyway.

I agree with you, but it doesn't sound like it would have helped things in this domain, as a negative index into a string (if they used signed ints and subtracted larger values) would have produced (according to the spec, undefined) values that were likely just as wrong.
There was a check for a negative difference before the array access, so signed arithmetic would have hit that check. Well, at least for any reasonable definition of signed arithmetic.
Doesn’t it make more sense to model counting of string length using natural numbers instead of integers?

It’s sort of like a dependent type which ensures you cannot represent invalid values.

I think that only works if your language traps underflow. In a language like C++ where unsigned integers silently wrap around, you’re not really ensuring that you cannot represent invalid values, merely that you represent invalid values as valid values.
The irony here is that the CPU will set the underflow making this trivial to detect in ASM with the `jb` instruction. The equivalent C++ would be something like:

    if (new_len > child.length())
As stated in the article, checking if an unsigned is less than 0 is a silly logic error but obviously one that is understandable to make. Still it's the sort of thing you'd expect automated tools would be able to catch.
The problem is being written as <= 0. The compiler could complain "why aren't you just == 0 because it is unsigned" but that's probably enabled at -Wpedantic or such level.
That's a good thing: the cliff is visible (zero is familiar) and intuitive (negative numbers are unsupported).

The pitfalls of signed arithmetic are much more insidious. Left shift a negative value? UB. Negate a negative value: maybe it's still negative. INT_MIN / -1? Crash!

Even basic arithmetic operations, e.g. the average of two integers, is unreasonably hard with signed arithmetic.

>Even basic arithmetic operations, e.g. the average of two integers, is unreasonably hard with signed arithmetic.

What do you have in mind? I understand that the addition can overflow, but that can happen with unsigned as well.

should have done it as a postgres ltree haha
The most interesting thing here is that this took down the bastion boxes that allowed PROD SSH access. I would argue that was the biggest red flag by far. As the author points out, if you can't access the machines somehow, you are pretty screwed. Great job that you managed to get back in anyway, but really the production access mechanism should be as dumb, simple, and standalone as you can make it.

We use multiple bastion hosts with user accounts provisioned by Ansible. There is one in each data center, plus a "shell" box at AWS that lives outside the firewall and also has SSH access to everything we need. All the bastions can access every server.

Plus, some of the mission-critical boxes actually have SSH exposed to the public internet. I would much rather take the security risk with (key-only) OpenSSH than the risk that I get locked out of PROD when I need to fix things after a 2am page.

It is entirely possible to make a box so secure that you yourself cannot access it, while not actually doing much to defend against more "reasonable" security risks, like typo-squatting and spear-phishing. It's all about the threat model.

Your perspective on the less secure but accessible public internet SSH box is interesting to me. It's as if you could wire up a weaker entryway with a ton of alarms of all kinds and probably feel pretty secure that way.
Heck, you could have an actual siren connected to the USB port! It'd be annoying enough that people would investigate if anyone tried anything.
Agreed, I wish the author would have explained why the authentication system was tied in so directly to this bug. Seems like the biggest learning would have been to separate the two.
> learning

I'm confused, did you mean lesson?

TFA did explain it. The boxen were authenticating against a LDAP server in prod, and prod was dead. This was obviously a huge design flaw and the author identified it as such.
I'm surprised compiler warnings haven't come up yet in this discussion. Checking if an unsigned number is smaller than zero typically triggers one, and code style tools can be set to treat any such check as a compilation error/not allow code with such a check to be checked in.