Hi Paul, On Tue, 2023-11-14 at 08:12 -0800, Paul Pluzhnikov wrote: > On Tue, Nov 14, 2023 at 4:57 AM Mark Wielaard <m...@klomp.org> wrote: > > > Urgh, I had no idea NULL + ... was technically undefined behavior. > > ISO/IEC 9899:201x > 6.5.6p8 > > When an expression that has integer type is added to or subtracted > from a pointer, the result has the type of the pointer operand. If the > pointer operand points to an element of an array object, and the array > is large enough, the result points to an element offset from the > original element such that the difference of the subscripts of the > resulting and original array elements equals the integer expression. > ... If both the pointer operand and the result point to elements of > the same array object, or one past the last element of the array > object, the evaluation shall not produce an overflow; otherwise, the > behavior is undefined.
Aha, so since NULL isn't a pointer to an array object a compiler may assume the pointer isn't NULL and possibly remove any NULL checks later in the code if it sees you do pointer arithmetic... boo. > > It would be slightly nicer if > > we could just do the computation after checking map_address != NULL > > (since ehdr is only used after such a check). That would require > > rearranging some of the if statements. Does that make the code too > > complicated? > > I tried it, and it does: we need both "map_addr != 0" and "ehdr is > properly aligned", but we can't compute the latter before evaluating > the former, and we have the else clause when either condition fails. I > can fix this with a goto, or a helper variable, but the current > solution of keeping ehdr as uintptr_t is much simpler. > > It also reduces the casting and line wrapping required :-) Yeah, I was afraid of that. Thanks for looking into it though. > > Also this only resolves the issue for the 64bit ELF case. Just above > > this code is basically the same code for 32bit ELF. That code also > > needs to be fixed. > > Sorry I missed that. I am slightly surprised our testsuite didn't catch this. We do have -- enable-sanitize-undefined which does build everything with -- sanitize=undefined. Which should enable -fsanitize=pointer-overflow. But I just tried (with gcc) and it seems to not trigger. > Revised patch attached. Looks good. Applied. Thanks, Mark
