Open Bug 1367146 (ubsan) Opened 3 years ago Updated 9 days ago

[meta] UBSAN errors

Categories

(Core :: General, task, P3)

task

Tracking

()

UNCONFIRMED

People

(Reporter: mliska, Unassigned)

References

(Depends on 33 open bugs)

Details

(Keywords: meta, Whiteboard: [js:tech-debt])

Attachments

(3 files)

Attached file firefox.ubsan.log
User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/58.0.3029.110 Safari/537.36

Steps to reproduce:

Build trunk with -fsanitize=undefined. It's a follow up of: https://bugzilla.mozilla.org/show_bug.cgi?id=1165904 and it will probably need to split it by components.
Component: Untriaged → General
Product: Firefox → Core
This is related to this SuSE bug: https://bugzilla.opensuse.org/show_bug.cgi?id=1040105

Some of these errors may be causing Firefox not to work when built with newer GCC versions, particularly with -O3 and autovectorization (alignment bugs...)
Naveed - wasn't sure it should go here. If not, please provide some guidance on how to handle this.
Component: General → JavaScript Engine
Flags: needinfo?(nihsanullah)
Summary: UBSAN errors → [meta] UBSAN errors
The log appears to be for all of gecko, no just JavaScript. 

Most of what I see are alignment issues that could be performance hazards. This may be worth a more detailed look to see if there are other hazards lurking.
Flags: needinfo?(nihsanullah)
Priority: -- → P3
Whiteboard: [js:tech-debt]
Depends on: 1400081
Depends on: 1400148
Core::General is a reasonable place for this because the complains are all over the codebase.

UBSAN reports are a mixed bag. Some of them are useful, but it tends to produce lots of complains about things that don't seem important. E.g. see bug 996026 comment 1.

I've fixed a couple of problems from the attachment in dependent bugs. A lot of the remainder are alignment issues, which seem harmless, but there are a couple of non-alignment problems that look more interesting. However, the log is now a few months old and many of the line numbers are out-of-date.

Martin, if it's not hard for you, would you mind doing another run and posting updated results, along with the revision that you used? Thank you.
Component: JavaScript Engine → General
Flags: needinfo?(mliska)
Depends on: 1401145
Depends on: 1401191
Attached file Current UBSAN log
I'm sending updated UBSAN log for revision:

commit 519bb0922bcfe5f0779ef8ae2518f959aac09448 (origin/master, origin/HEAD)
Merge: 1e48981df6a1 bcef780a7796
Author: Wes Kocher <wkocher@mozilla.com>
Date:   Mon Sep 18 16:21:01 2017 -0700

    Merge inbound to central, a=merge
    
    MozReview-Commit-ID: EK8iFR1hSRp
Flags: needinfo?(mliska)
Filed https://ssl.icu-project.org/trac/ticket/13362 for the UBSan errors in the imported ICU library.
Depends on: 1401200
Depends on: 1401209
Thanks Andree for taking that seriously. Tomorrow I'll send also updated version for ASAN.
Anyhow, I really believe that project like Fifefox should build such builds periodically and run test-suite with them.
I can help with question related to that.
> commit 519bb0922bcfe5f0779ef8ae2518f959aac09448 (origin/master, origin/HEAD)
> Merge: 1e48981df6a1 bcef780a7796
> Author: Wes Kocher <wkocher@mozilla.com>
> Date:   Mon Sep 18 16:21:01 2017 -0700
> 
>     Merge inbound to central, a=merge
>     
>     MozReview-Commit-ID: EK8iFR1hSRp

That looks like a revision from a git repo?

On mozilla-inbound this is revision 30a386ff1192.
I went through the "Current UBSAN log", removed duplicates, added some comments, and sorted them in rough order of interest.
Martin, a few more questions:

- How did you build Firefox?

- How did you run Firefox, and what workload did you exercise it on?

- The original log had lots of complaints about misaligned addresses. Do you know why they aren't present in the latest log?

Thank you.
Flags: needinfo?(mliska)
I build it with:

$ cat .mozconfig 
# Build Firefox with: OPT="-O2" MYFLAGS="-flto=9" make -f client.mk build

mk_add_options MOZ_MAKE_FLAGS="-j9"

MYFLAGS="$OPT $MYFLAGS"
mk_add_options OS_CFLAGS="$MYFLAGS"
mk_add_options OS_CXXFLAGS="$MYFLAGS"
mk_add_options OS_LDFLAGS="$MYFLAGS"
ac_add_options --enable-optimize=$OPT

ac_add_options --enable-application=browser
ac_add_options --disable-tests
ac_add_options --disable-debug
ac_add_options --disable-debug-symbols
# ac_add_options --enable-system-icu

# valgrind purpose
ac_add_options --disable-jemalloc
ac_add_options --disable-valgrind

# ac_add_options --disable-ogg
# ac_add_options --with-system-libvpx
# ac_add_options --disable-necko-wifi
ac_add_options --disable-elf-hack

export CFLAGS="$MYFLAGS"
export CXXFLAGS="$MYFLAGS"
export LDFLAGS="$MYFLAGS"

$ OPT="-O2" MYFLAGS="-fsanitize=undefined -ldl -flifetime-dse=1 -fno-sanitize=alignment" nice make -f client.mk build
...

I basically visit couple of pages and I run http://browserbench.org/JetStream/.

It's missing because I disabled this kind of errors.
Flags: needinfo?(mliska)
Thank you for the info. The updated log is actually pretty good -- there are several things I plan to investigate, and not too much in the way of annoying or silly stuff.
Depends on: 1401515
I just saw that there is an existing bug for UBSan issues in SpiderMonkey (bug 1284975), I've added an additional list of UBSan errors when running the SpiderMonkey test suites to that bug report.
About this one:

> /home/marxin/Programming/gecko-dev/obj-x86_64-pc-linux-gnu/dist/include/mozilla/ipc/ProtocolUtils.h:626:22: runtime error: load of value 32764, which is not a valid value for type 'Mode'

It's a false positive, more or less. Specifically, it's true that EndPoint::mMode is set to an invalid value, but there's another field EndPoint::mValid which is always false when that happens, and mMode is never read while mValid is false.

One option is to set mMode in the constructor even when mValid is false, which should satisfy UBSan. The downside of this is that it will prevent Valgrind from detecting any use of mMode before it's set properly.
About this one:

> /home/marxin/Programming/gecko-dev/gfx/layers/apz/util/InputAPZContext.cpp:58:18: runtime error: load of value 2463969997, which is not a valid value for type 'nsEventStatus'

Again, looks like a false positive. This code calls the constructor with an uninitialized argument:

http://searchfox.org/mozilla-central/rev/2ef8bd8a46a02c68ddbb1d5f25fa254dd7be1fbd/dom/ipc/TabChild.cpp#1684-1685

but that argument appears to never be used.

Valgrind doesn't complain about uninitialized values until they are used in a way that could affect program execution. Unfortunately UBSan complains about some such values simply when they are copied, which is a recipe for false positives.
Should we add a MOZ_UBSAN_SILENCE("type") that will suppress the items you've identified as false positives? I think I can handle that for the couple you've noted in #14 and #15

https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#disabling-instrumentation-with-attribute-no-sanitize-undefined
Flags: needinfo?(n.nethercote)
(In reply to Tom Ritter [:tjr] from comment #16)
> Should we add a MOZ_UBSAN_SILENCE("type") that will suppress the items
> you've identified as false positives? I think I can handle that for the
> couple you've noted in #14 and #15

That sounds like a good idea -- it will placate UBSan in a way that doesn't reduce Valgrind's effectiveness.
Flags: needinfo?(n.nethercote)
Agree that MOZ_UBSAN_SILENCE is a good idea, however note that GCC only supports that on functions, not on types or fields of a type.
Alias: ubsan
Depends on: 1404151
Depends on: 1404169
Depends on: 1404174
Depends on: 1404189
Depends on: 1404219
Depends on: 1404220
Depends on: 1404224
Depends on: 1404226
Depends on: 1404547
Depends on: 1404570
Depends on: 1404572
Depends on: 1469410
Depends on: 1469910
No longer depends on: 1469410
Depends on: 1467920
Depends on: 1460764
Depends on: 1478523
Depends on: 1436778
Depends on: 1284975
Depends on: 1532849
Depends on: 1532858
Depends on: 1431882
Depends on: 1532861
Depends on: 1532867
Depends on: 1532868
Depends on: 1413750
Depends on: 1533127
Depends on: 1533612
Depends on: 1534156
Depends on: 1534709
Depends on: 1535980
Depends on: 1538768
Type: defect → task
Depends on: 1439446, 1438260
Depends on: 1468131
Depends on: 1568047
Depends on: 1573049
Depends on: 1575584
Depends on: 1577584
Depends on: 1577669
Depends on: 1580352
Depends on: 1580317
Depends on: 1438948
Depends on: 1580918
Depends on: 1581655
Depends on: 1581672
Depends on: 1581946
Depends on: 1581964
Depends on: 1581986
Depends on: 1583291
Depends on: 1583293
Depends on: 1583399
Depends on: 1583402
Depends on: 1447055
Depends on: 1583405
Depends on: 1583645
Depends on: 1583946
Depends on: 1583967
Depends on: 1583970
Depends on: 1583995
Depends on: 1584005
Depends on: 1584006
Depends on: 1584008
Depends on: 1584407
Depends on: 1584601
Depends on: 1584639
Depends on: 1584640
Depends on: 1584643
Depends on: 1584660
Depends on: 1585721
Depends on: 1585845
Depends on: 1586165
Depends on: 1586170
Depends on: 1587146
Depends on: 1587159
Depends on: 1587162
Depends on: 1587164
Depends on: 1587173
Depends on: 1587176
Depends on: 1588938
Depends on: 1588940
Depends on: 1589496
Depends on: 1589527
Depends on: 1593387
Depends on: 1594942
Depends on: 1595259
Depends on: 1595260
You need to log in before you can comment on or make changes to this bug.