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.
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.
if (!s) puts("hi");
strlen(s);
More fun is memset(0,0,0) which is technically verboten.
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.