Don't add to NULL in iterator. #398
Conversation
In C it is undefined to add anything to NULL. Clang recently began taking advantage of this and can assume that if anything is added or subtracted from a pointer that the pointer can be assumed non-NULL. The Address Sanitizer has been updated to report when this happens at runtime and produces messages like expat/lib/xmlparse.c:6509:23: runtime error: applying zero offset to null pointer SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior expat/lib/xmlparse.c:6509:23 This can be mitigated with 'p ? p + n : NULL' which optimizes to just the add in all optimizing compilers, but avoids the undefined behavior.
There was a problem hiding this comment.
Hi @bungeman , thanks for raising awareness about this issue. I'll need to understand the issue better to be able to evaluate if this is a good approach to a fix or not. Have you traced back where NULL iter->p can be resulting from and what the consequences of NULL iter->end will be, already?
|
I honestly haven't looked too hard, but this currently seems to happen any time initializing an iterator on a hashTable which hasn't had anything added to it. Note that FreeType had a number of similar issues and ended up adding a FT_OFFSET macro for this, it's probably best to clone freetype from git://git.sv.nongnu.org/freetype/freetype2.git and do 'git log -SFT_OFFSET' for the recent history and how it is used there. It may be worth it to do that here, both for future reference and documentation. |
|
I have had a closer look at the related code and your suggested fix now. I agree there is a problem and also agree that this is the right move. Thanks for your contribution to libexpat! 👍 |
drop ubsan patch in favour of fix applied as libexpat/libexpat#398 Change-Id: I59eb9e24206b9a4cf323b7f7d48d8df0792a1c46 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/116102 Tested-by: Jenkins Reviewed-by: Michael Stahl <michael.stahl@allotropia.de> (cherry picked from commit 740d12d) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119422 Tested-by: Thorsten Behrens <thorsten.behrens@allotropia.de> Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>
drop ubsan patch in favour of fix applied as libexpat/libexpat#398 Change-Id: I59eb9e24206b9a4cf323b7f7d48d8df0792a1c46 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/116102 Tested-by: Jenkins Reviewed-by: Michael Stahl <michael.stahl@allotropia.de> (cherry picked from commit 740d12d) Reviewed-on: https://gerrit.libreoffice.org/c/core/+/119426 Tested-by: Thorsten Behrens <thorsten.behrens@allotropia.de> Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>
drop ubsan patch in favour of fix applied as libexpat/libexpat#398 Change-Id: I59eb9e24206b9a4cf323b7f7d48d8df0792a1c46 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/116102 Tested-by: Jenkins Reviewed-by: Michael Stahl <michael.stahl@allotropia.de> (cherry picked from commit 740d12d)
drop ubsan patch in favour of fix applied as libexpat/libexpat#398 Change-Id: I59eb9e24206b9a4cf323b7f7d48d8df0792a1c46 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/116102 Tested-by: Jenkins Reviewed-by: Michael Stahl <michael.stahl@allotropia.de> (cherry picked from commit 740d12d)
In C it is undefined to add anything to NULL. Clang recently began
taking advantage of this and can assume that if anything is added or
subtracted from a pointer that the pointer can be assumed non-NULL. The
Address Sanitizer has been updated to report when this happens at
runtime and produces messages like
expat/lib/xmlparse.c:6509:23: runtime error: applying zero offset to null pointer
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior expat/lib/xmlparse.c:6509:23
This can be mitigated with 'p ? p + n : NULL' which optimizes to just
the add in all optimizing compilers, but avoids the undefined behavior.