1401200 - Don't call qsort with nullptr in jit::AnalyzeNewScriptDefiniteProperties

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[André Bargull [:anba]]

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

Comment 1

[André Bargull [:anba]]

Attachment: bug1401200.patch

Another easy UBSan error to fix.

Comment 2

[Nicolas B. Pierron [:nbp]]

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?

Comment 3

[André Bargull [:anba]]

(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

Comment 4

[Nicolas B. Pierron [:nbp]]

(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.

Comment 5

[André Bargull [:anba]]

(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! :-)

Comment 6

[André Bargull [:anba]]

(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 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8909792 [details] [diff] [review]
bug1401200.patch

Review of attachment 8909792 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling as mentioned in previous comments and IRC.

Comment 8

[André Bargull [:anba]]

Attachment: bug1401200.patch

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

Comment 9

[André Bargull [:anba]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8473d94208f5
Don't call qsort with nullptr in jit::AnalyzeNewScriptDefiniteProperties. r=nbp

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/8473d94208f5