Open Bug 1402316 Opened 7 years ago Updated 2 years ago

Silence UBSAN errors that don't apply to us

Categories

(Core :: General, enhancement)

enhancement

Tracking

()

People

(Reporter: tjr, Unassigned)

References

(Blocks 1 open bug)

Details

In Bug 1367146 Comments 14 and 15 there are a few UBSAN errors we'd like to silence
I recognize the two errors mentioned in that ticket as detected by `gcc -fsanitize=enum`. In both cases the code is reading the value of an uninitialized enum type. There are about a dozen enum errors I found in the codebase by running all mozilla-central tests on linux with -fsanitize=undefined. (The overall table including other ubsan runtime errors is at https://docs.google.com/spreadsheets/d/1ISxhkwWVwa7HBVEd6gPTcynfMwaq-cmI_wQsiDZxLhc/edit#gid=1093183829) I currently have patches to fix nine of the enum errors here: https://github.com/arthuredelstein/tor-browser/commits/ubsan-enum2 These include the two mentioned in the Description. I'm hoping to write patches for the last three or so soon. My feeling is these enum cases are programming errors that are usually harmless but occasionally could produce unintended behavior. So I think these are better to fix the rest of these patches so we can turn this check on in the debug build and catch any more such future errors before they land. Anyway, curious if you think I'm on the right track here.
Hi Nicholas-- I had been working on patching these fsanitize=enum runtime errors but since saw your comments in bug 1367146. I'm not very familiar with what valgrind catches. What's your opinion on my approach in this branch? https://github.com/arthuredelstein/tor-browser/commits/ubsan-enum2 Thanks.
Flags: needinfo?(n.nethercote)
Valgrind only complains when you use an uninitialized value in a way that can visibly affect the program's execution, primarily: - in the condition of a conditional branch; - as an address in a memory load or store; - as an input to a system call. Valgrind won't complain in the following common cases: - when an uninitialized value is copied; - when an uninitialized value is used in arithmetic. In the latter two cases, Valgrind will of course track the fact that the resulting values are also uninitialized, and it does this with bit-level precision. (http://www.valgrind.org/docs/memcheck2005.pdf has more details if you are interested.) In contrast, UBSan does complain when an uninitialized-and-hence-invalid enum value is copied, even though these are often harmless. As for fixing these problems, one good approach is to use the mozilla::Maybe type, defined in mfbt/Maybe.h, as I did in bug 1400081. But sometimes mozilla::Maybe is awkward to use, and so introducing an "invalid" value to an enum and using that for initialization one reasonable approach. E.g. I did that with nsCursor in bug 1400148 and you did the same thing in your branch. A bad approach that should be avoided is to just use an existing valid enum value, because that's basically just lying to satisfy UBSan.
Flags: needinfo?(n.nethercote)
No longer blocks: 1404547
Status: NEW → ASSIGNED
Assignee: tom → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.