Closed Bug 1489979 Opened 2 years ago Closed 2 years ago

IsDependentOn relies on Undefined Behavior

Categories

(Core :: String, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: hsivonen, Assigned: hsivonen)

Details

Attachments

(1 file, 1 obsolete file)

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.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
MozReview-Commit-ID: EHEIFvmFOLc
Huh. The patch breaks all opt builds, so it seems to have the opposite effect of what it's supposed to have.
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 .)
(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.
Attachment #9008013 - Attachment is obsolete: true
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
The source of my confusion was bug 1487341, which, it turns out, fails in opt builds.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
Comment on attachment 9009596 [details]
Bug 1489979 - Avoid UB in IsDependentOn().

Nathan Froyd [:froydnj] has approved the revision.
Attachment #9009596 - Flags: review+
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/380cac58bbc5
Avoid UB in IsDependentOn(). r=froydnj
https://hg.mozilla.org/mozilla-central/rev/380cac58bbc5
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.