4 ms·

> 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 ope

> 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.

tedunangst 8y ago
Calling string functions on null pointers is mostly undefined, which can lead to all the same problems as dereferencing. Notably the compiler can now delete "extraneous" null checks.
- scottlamb 8y ago
  Makes sense. Do you happen to know of any way this can cause null pointer dereference to result in something other than SIGSEGV in a "normal" situation (userspace, modern POSIX system, modern hardware, no prior mmap(NULL, ..., MAP_FIXED, ...) call, no huge offset into the null pointer)? I can see how it would if you relax any of those conditions. In particular, I see an example of the deleted null check you're describing here: https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/ In any case, I'd still say strlen(NULL) is qualitatively different than check_ip_header_sum(malloc(4095), 4095). The former's undefined behavior is standard (strlen is expected to dereference the pointer, so it makes sense that it has whatever problems doing that has). The latter shouldn't be expected to exceed the stated bounds without that being clearly stated.
  - tedunangst 8y ago
    strlen(null) is pretty straightforward, although being a pure function the compiler might rearrange things. if (!s) puts("hi"); strlen(s); I don't think that's guaranteed to print hi if s is null. More fun is memset(0,0,0) which is technically verboten.
klodolph 8y ago
> 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. The function is already full of UB as we know it. You can add one more UB to the pile if you like. > I take a pretty hard stance on this kind of thing. That’s a valid choice that you can make, as is writing code in Rust. But what’s you’re doing is criticizing the design of an API that you have no actual knowledge of. For all we know this function is an internal function, not exported for use elsewhere, buried in some other code which performs the appropriate checks. We know, from the article, that other checks are performed elsewhere in the code, because it’s explicitly mentioned. “Taking a hard stance” is all fine when you are writing or reviewing code, but in this case this kind of criticism is just noise. Save it for when you see a complete enough picture that your disagreements are more than just stylistic.

tedunangst 8y ago

Calling string functions on null pointers is mostly undefined, which can lead to all the same problems as dereferencing. Notably the compiler can now delete "extraneous" null checks.

scottlamb 8y ago
Makes sense. Do you happen to know of any way this can cause null pointer dereference to result in something other than SIGSEGV in a "normal" situation (userspace, modern POSIX system, modern hardware, no prior mmap(NULL, ..., MAP_FIXED, ...) call, no huge offset into the null pointer)? I can see how it would if you relax any of those conditions. In particular, I see an example of the deleted null check you're describing here: https://lwn.net/Articles/342330/ https://lwn.net/Articles/342330/ In any case, I'd still say strlen(NULL) is qualitatively different than check_ip_header_sum(malloc(4095), 4095). The former's undefined behavior is standard (strlen is expected to dereference the pointer, so it makes sense that it has whatever problems doing that has). The latter shouldn't be expected to exceed the stated bounds without that being clearly stated.
- tedunangst 8y ago
  strlen(null) is pretty straightforward, although being a pure function the compiler might rearrange things. if (!s) puts("hi"); strlen(s); I don't think that's guaranteed to print hi if s is null. More fun is memset(0,0,0) which is technically verboten.

klodolph 8y ago

> 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. The function is already full of UB as we know it. You can add one more UB to the pile if you like. > I take a pretty hard stance on this kind of thing. That’s a valid choice that you can make, as is writing code in Rust. But what’s you’re doing is criticizing the design of an API that you have no actual knowledge of. For all we know this function is an internal function, not exported for use elsewhere, buried in some other code which performs the appropriate checks. We know, from the article, that other checks are performed elsewhere in the code, because it’s explicitly mentioned. “Taking a hard stance” is all fine when you are writing or reviewing code, but in this case this kind of criticism is just noise. Save it for when you see a complete enough picture that your disagreements are more than just stylistic.