3 ms·

Can't this also segfault by overreading across a page boundary? It's assuming "size" is >= 20 and is a multiple of 4. That might be true for all valid headers,

Can't this also segfault by overreading across a page boundary? It's assuming "size" is >= 20 and is a multiple of 4. That might be true for all valid headers, but at least in this snippet, there's no comment or anything stating the precondition that callers ensure the headers are valid. That's a(nother) bug. If it's not true, it will read too much data. This means producing the wrong result. It also could make the read happen to cross a page boundary into space that isn't mapped into the process's virtual address space.

klodolph 8y ago
Depending on which page boundary, increasingly severe exceptions will be raised, but it is typical for an OS to service those exceptions by performing the operation and jumping back into the code… maintaining the illusion that it worked. This is what happened to all unaligned accesses in the PowerPC 601 -> 603e transition in ~1995. Game developers writing their own “optimized” blit code suddenly found that every single unaligned access on the 603 would succeed, but it would go through an ISR to get there! I feel for these kind of routines, either write it in dumb, straightforward C code and just use -O2 or -Os, or make an assembler version—possibly just a saved copy of the compiler’s output, but that’s enough to completely kill any interprocedural optimization. Trying to write ultra-optimized C code gives me a bit of a headache, unless it’s something that you can express straightforwardly as SSE intrinsics or something like that. You end up going through contortions to satisfy constraints imposed by the language which aren’t imposed by the machine. > It's assuming "size" is >= 20 and is a multiple of 4. That might be true for all valid headers, but at least in this snippet, there's no comment or anything stating the precondition that callers ensure the headers are valid. That's a(nother) bug. That’s too much of a reach to call it a bug. The IP header’s size is measured in multiples of 4 bytes, there is simply no way to encode a header which is not a multiple of 4 bytes. It becomes a question of how gracefully you want to handle true garbage being passed into the function… at best this is a matter of style. Should strlen() call assert(p!=NULL)? These days with asan, I’m not so sure that writing these assert() is the best way to spend my time.
- scottlamb 8y ago
  > Depending on which page boundary, increasingly severe exceptions will be raised, but it is typical for an OS to service those exceptions by performing the operation and jumping back into the code… maintaining the illusion that it worked. I'd expect crossing a boundary between two valid pages to be fine; that's just part of making unaligned accesses work. I'm talking specifically about an unmapped page. I don't have any data, but I'm pretty confident access to an unmapped page (such as null pointers) is a much more common source of SIGSEGV in general than unaligned access, to the extent that most programmers probably don't even know the latter is a possibility. > That’s too much of a reach to call it a bug. The IP header’s size is measured in multiples of 4 bytes, there is simply no way to encode a header which is not a multiple of 4 bytes. I take a pretty hard stance on this kind of thing. The function might be used in a way you're not expecting. The article mentioned the IP frames can be read from a file. Maybe they're in some record-delimited format that states length in bytes, and that's the length that gets passed in rather than the original one. Redundant framing information like that is pretty common I think when you're using some general-purpose code (a library that handles a variety of record formats as well as some functions that operate on specific types like IP frames). Those records could get corrupted, even maliciously so; callers probably expect then to get back a nonsense result then but not an out-of-bounds read. Or there could be a bug in the logic that decodes the header's length. By itself that won't cause memory errors, but combined with this assumption it does, and so I'm uncomfortable with the assumption being implicit. I think a lot of C code's memory safety is dependent on this sort of implicit assumption that rots over time (if it was ever correct to begin with), and every time I see it I prefer memory-safe languages a little more. Working in a language (Rust) that requires you to explicitly annotate such places with "unsafe" and have your public interfaces be "sound" (have it be impossible for safe code to cause the unsafe code to do the wrong thing) makes this more clear to me. > Should strlen() call assert(p!=NULL)? If it behaves in a surprising way when called with NULL, it should mention that case (either in an interface comment/manpage or via explicit assert). In practice on modern machines strlen raises SIGSEGV. [1] When folks see a SIGSEGV from strlen, they probably think wild pointer or missing terminator and will figure out exactly what happened before long. [1] Although I imagine it's actually undefined behavior according to the C standard, so now I wonder if some compiler does / my compiler will someday do something more strange, and I'm sad.
caf 8y ago
It does actually say that: > In addition, the size can never be less than that – this is checked before calling this function.

klodolph 8y ago

Depending on which page boundary, increasingly severe exceptions will be raised, but it is typical for an OS to service those exceptions by performing the operation and jumping back into the code… maintaining the illusion that it worked. This is what happened to all unaligned accesses in the PowerPC 601 -> 603e transition in ~1995. Game developers writing their own “optimized” blit code suddenly found that every single unaligned access on the 603 would succeed, but it would go through an ISR to get there! I feel for these kind of routines, either write it in dumb, straightforward C code and just use -O2 or -Os, or make an assembler version—possibly just a saved copy of the compiler’s output, but that’s enough to completely kill any interprocedural optimization. Trying to write ultra-optimized C code gives me a bit of a headache, unless it’s something that you can express straightforwardly as SSE intrinsics or something like that. You end up going through contortions to satisfy constraints imposed by the language which aren’t imposed by the machine. > It's assuming "size" is >= 20 and is a multiple of 4. That might be true for all valid headers, but at least in this snippet, there's no comment or anything stating the precondition that callers ensure the headers are valid. That's a(nother) bug. That’s too much of a reach to call it a bug. The IP header’s size is measured in multiples of 4 bytes, there is simply no way to encode a header which is not a multiple of 4 bytes. It becomes a question of how gracefully you want to handle true garbage being passed into the function… at best this is a matter of style. Should strlen() call assert(p!=NULL)? These days with asan, I’m not so sure that writing these assert() is the best way to spend my time.

scottlamb 8y ago
> Depending on which page boundary, increasingly severe exceptions will be raised, but it is typical for an OS to service those exceptions by performing the operation and jumping back into the code… maintaining the illusion that it worked. I'd expect crossing a boundary between two valid pages to be fine; that's just part of making unaligned accesses work. I'm talking specifically about an unmapped page. I don't have any data, but I'm pretty confident access to an unmapped page (such as null pointers) is a much more common source of SIGSEGV in general than unaligned access, to the extent that most programmers probably don't even know the latter is a possibility. > That’s too much of a reach to call it a bug. The IP header’s size is measured in multiples of 4 bytes, there is simply no way to encode a header which is not a multiple of 4 bytes. I take a pretty hard stance on this kind of thing. The function might be used in a way you're not expecting. The article mentioned the IP frames can be read from a file. Maybe they're in some record-delimited format that states length in bytes, and that's the length that gets passed in rather than the original one. Redundant framing information like that is pretty common I think when you're using some general-purpose code (a library that handles a variety of record formats as well as some functions that operate on specific types like IP frames). Those records could get corrupted, even maliciously so; callers probably expect then to get back a nonsense result then but not an out-of-bounds read. Or there could be a bug in the logic that decodes the header's length. By itself that won't cause memory errors, but combined with this assumption it does, and so I'm uncomfortable with the assumption being implicit. I think a lot of C code's memory safety is dependent on this sort of implicit assumption that rots over time (if it was ever correct to begin with), and every time I see it I prefer memory-safe languages a little more. Working in a language (Rust) that requires you to explicitly annotate such places with "unsafe" and have your public interfaces be "sound" (have it be impossible for safe code to cause the unsafe code to do the wrong thing) makes this more clear to me. > Should strlen() call assert(p!=NULL)? If it behaves in a surprising way when called with NULL, it should mention that case (either in an interface comment/manpage or via explicit assert). In practice on modern machines strlen raises SIGSEGV. [1] When folks see a SIGSEGV from strlen, they probably think wild pointer or missing terminator and will figure out exactly what happened before long. [1] Although I imagine it's actually undefined behavior according to the C standard, so now I wonder if some compiler does / my compiler will someday do something more strange, and I'm sad.

caf 8y ago

It does actually say that: > In addition, the size can never be less than that – this is checked before calling this function.