IsDependentOn relies on Undefined Behavior

RESOLVED FIXED in Firefox 64

Status

()

enhancement
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: hsivonen, Assigned: hsivonen)

Tracking

unspecified
mozilla64
Points:
---

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

8 months ago
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)

Updated

8 months ago
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 2

8 months ago
MozReview-Commit-ID: EHEIFvmFOLc
(Assignee)

Comment 3

8 months ago
Huh. The patch breaks all opt builds, so it seems to have the opposite effect of what it's supposed to have.
(Assignee)

Comment 4

8 months ago
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 .)
(Assignee)

Comment 5

8 months ago
(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)

Updated

8 months ago
Assignee: hsivonen → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 6

8 months ago
The source of my confusion was bug 1487341, which, it turns out, fails in opt builds.
Assignee: nobody → hsivonen
Status: NEW → ASSIGNED
(Assignee)

Comment 8

8 months ago
MozReview-Commit-ID: C6ehO1TG5YO
Comment on attachment 9009596 [details]
Bug 1489979 - Avoid UB in IsDependentOn().

Nathan Froyd [:froydnj] has approved the revision.
Attachment #9009596 - Flags: review+

Comment 11

8 months ago
Pushed by hsivonen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/380cac58bbc5
Avoid UB in IsDependentOn(). r=froydnj

Comment 12

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/380cac58bbc5
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.