1489979 - IsDependentOn relies on Undefined Behavior

Status: RESOLVED FIXED

(Core :: XPCOM, enhancement)

Description

[Henri Sivonen (:hsivonen)]

https://searchfox.org/mozilla-central/source/xpcom/string/nsTStringRepr.h#295 says:
  // Returns true if this string overlaps with the given string fragment.
  bool IsDependentOn(const char_type* aStart, const char_type* aEnd) const
  {
     // If it _isn't_ the case that one fragment starts after the other ends,
     // or ends before the other starts, then, they conflict:
     //
     //   !(f2.begin >= f1.aEnd || f2.aEnd <= f1.begin)
     //
     // Simplified, that gives us:
    return (aStart < (mData + mLength) && aEnd > mData);
  }

This is UB if the arguments come from a different allocation than mData. I'm not aware of any practical problem right now, but as we use LTO more, which gives the optimizer more visibility into the code, it's a bit worriesome that in principle, the above asserts to the compiler that aStart and mData come from the same allocation, which in principle could be used to transform some other code.

We should use std::less and std::greater instead of < and > to avoid UB here, just in case.

Comment 1

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e3e0aa75658fcdf9d1dcd54a7cd046dba14a1b3

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1489979 - Avoid UB in IsDependentOn().

MozReview-Commit-ID: EHEIFvmFOLc

Comment 3

[Henri Sivonen (:hsivonen)]

Huh. The patch breaks all opt builds, so it seems to have the opposite effect of what it's supposed to have.

Comment 4

[Henri Sivonen (:hsivonen)]

With implemenation-defined behavior instead of Undefined Behavior:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a008bee3a31241165387ba217d13a8264392a29d

(This might be placebo at present given bugs shown in appendices A and B of https://www.cs.utah.edu/~regehr/oopsla18.pdf .)

Comment 5

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #4)
> With implemenation-defined behavior instead of Undefined Behavior:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a008bee3a31241165387ba217d13a8264392a29d
> 
> (This might be placebo at present given bugs shown in appendices A and B of
> https://www.cs.utah.edu/~regehr/oopsla18.pdf .)

And that patch is very red and orange on treeherder. Now I'm confused.

Comment 6

[Henri Sivonen (:hsivonen)]

The source of my confusion was bug 1487341, which, it turns out, fails in opt builds.

Comment 7

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0cfbe46e8411141c90ba7bd58d41d5a719d6586b

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Bug 1489979 - Avoid UB in IsDependentOn().

MozReview-Commit-ID: C6ehO1TG5YO

Comment 9

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e610de9c8d216e5a615cb16b014ed8d510fe9e60

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 9009596 [details]
Bug 1489979 - Avoid UB in IsDependentOn().

Nathan Froyd [:froydnj] has approved the revision.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/380cac58bbc5
Avoid UB in IsDependentOn(). r=froydnj

Comment 12

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/380cac58bbc5