From bug 1367146: /home/marxin/Programming/gecko-dev/js/src/jit/IonAnalysis.cpp:4283:10: runtime error: null pointer passed as argument 1, which is declared to never be null

Another easy UBSan error to fix.

Comment on attachment 8909792 [details] [diff] [review] bug1401200.patch Review of attachment 8909792 [details] [diff] [review]: ----------------------------------------------------------------- How checking the length supposed to reassure UBSan that mBegin is non-null?

(In reply to Nicolas B. Pierron [:nbp] from comment #2) > Comment on attachment 8909792 [details] [diff] [review] > bug1401200.patch > > Review of attachment 8909792 [details] [diff] [review]: > ----------------------------------------------------------------- > > How checking the length supposed to reassure UBSan that mBegin is non-null? A non-empty Vector always has a non-nullptr mBegin, hasn't it? Details: |Vector<MInstruction*> instructions| doesn't specify a minimum inline capacity, so mfbt::Vector uses the non-inline storage specialization for CRAndStorage [1]. And that causes mfbt::Vector::mBegin to be initialized with nullptr. So when no entries are added to |instructions|, mBegin is still null when we call qsort. But when any entries were added to |instructions|, the Vector is converted to use heap storage, which changes mBegin to a non-nullptr value. [1] http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/mfbt/Vector.h#410-419 [2] http://searchfox.org/mozilla-central/rev/1c13d5cf85f904afb8976c02a80daa252b893fca/mfbt/Vector.h#930,946

(In reply to André Bargull from comment #3) > > How checking the length supposed to reassure UBSan that mBegin is non-null? > > A non-empty Vector always has a non-nullptr mBegin, hasn't it? Yes, but how is UBSan supposed to know the relation between the non-zero length and the non-nullptr mBegin? If UBSan were capable to make the link between the 2, it should be able to determine that it is safe to call qsort even when the first argument is a nullptr because the second argument is always zero. Another fix would be to add inline storage to the instruction vector.

(In reply to Nicolas B. Pierron [:nbp] from comment #4) > (In reply to André Bargull from comment #3) > > > How checking the length supposed to reassure UBSan that mBegin is non-null? > > > > A non-empty Vector always has a non-nullptr mBegin, hasn't it? > > Yes, but how is UBSan supposed to know the relation between the non-zero > length and the non-nullptr mBegin? > Ah, now I understand what you mean. The report was generated when running a Firefox build with "-fsanitize=undefined" enabled, so this isn't a static UB checker report. But please don't ask me to verify which standard requires that qsort must not be called a null pointer! :-)

(In reply to André Bargull from comment #5) > But please don't ask me to verify which standard requires > that qsort must not be called a null pointer! :-) Dammit. http://iso-9899.info/n1570.html#7.1.4 Each of the following statements applies unless explicitly stated otherwise in the detailed descriptions that follow: If an argument to a function has an invalid value (such as [...], or a null pointer, [...]) or [...], the behavior is undefined. And http://iso-9899.info/n1570.html#7.22.5 doesn't specify a special behaviour for null pointers when calling qsort.

Comment on attachment 8909792 [details] [diff] [review] bug1401200.patch Review of attachment 8909792 [details] [diff] [review]: ----------------------------------------------------------------- Canceling as mentioned in previous comments and IRC.

Updated patch to use an inline storage with an initial capacity of four elements, as suggested on IRC.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=870329d65fc96d37befc13269ffca15d95693ef1

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8473d94208f5 Don't call qsort with nullptr in jit::AnalyzeNewScriptDefiniteProperties. r=nbp