TSan: data races in js/src/vm/TypeInference.cpp TypeSet::TypeString(TypeSet::Type type)

RESOLVED FIXED in Firefox 58

Status

()

P3
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: jseward, Assigned: sfink)

Tracking

({triage-deferred})

unspecified
mozilla58
triage-deferred
Points:
---

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
This is a new race that has appeared within the past two weeks,
and is now the most frequently TSan-reported JS related race
when running m-c.

TypeSet::TypeString(TypeSet::Type type) uses a global variable |which|
to cycle through a 4-entry buffer |bufs[4][40]|, selecting one of the
four entries for use for that call.  Unfortunately TypeSet::TypeString
is called from multiple threads, so TSan reports a race on |which|.

Even if we made |which| be atomic, there would still be a race on
|bufs[.]|.  Imagine 5 threads calling this function simultaneously.
Two of them are going to wind up with the same |which| value and so
they will have a write-write race on |bufs[which][.]|.

What purpose does this circular buffer mechanism serve?  At least
from a quick scan of the rest of TypeInference.cpp, the strings returned
by this function are handed on immediately to various kinds of logging
or printing functions.  I didn't check uses outside TypeInference.cpp, though.
(Reporter)

Comment 1

2 years ago
Created attachment 8794134 [details]
TSan complaint
(Reporter)

Comment 2

2 years ago
Created attachment 8794143 [details]
TSan complaints for the w-w race on |bufs[which][.]|

TSan also detects the write-write race on |bufs[which][.]|.
sfink, you touched this stuff on September 13th (the most recent blame on the function :P), do you have any idea what's up with this?
Flags: needinfo?(sphink)
Keywords: triage-deferred
Priority: -- → P3
(Assignee)

Comment 4

a year ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #3)
> sfink, you touched this stuff on September 13th (the most recent blame on
> the function :P), do you have any idea what's up with this?

Ugh, this looks to be some kind of cheap hack to avoid heap-allocation for a temporary string, where a couple of strings' lifespans overlap. It looks like I moved the code in bug 1301764, but it dates back to bug 637674. It appears to intended to be DEBUG-only, though it is now enabled unconditionally now. Ah, it's used for InferSpew.

These should probably be fixed to pass in stack-allocated buffers or something.
Flags: needinfo?(sphink)
(Assignee)

Comment 5

a year ago
Oh, I see. Most of the time, these stringifying functions can get away with simply returning a const char* to a string literal. So the API is kind of built around that, and properly handling buffers is a little clunky for a debugging feature.
(Assignee)

Comment 6

a year ago
Ugh. And when taking a quick stab at it, I'm nervous about passing through stack allocated buffers everywhere, because it would bloat up the frame size of several hot type inference functions, and for all I know that might be a big deal.
(Assignee)

Comment 7

a year ago
Created attachment 8914528 [details] [diff] [review]
Remove race on TypeString() char buffers

This is the mindless, mechanical way of fixing it. But as I mentioned, it bloats up a bunch of stack frames, I'm ambivalent as to whether it's a good idea to land this and see whether it regresses anything. It would be better to do something fancy with returning a UniqueChars or something and discarding it at the right time, but I didn't want to take the time right now to design that out.
Attachment #8914528 - Flags: review?(jcoppeard)
(Assignee)

Updated

a year ago
Assignee: nobody → sphink
Status: NEW → ASSIGNED
Comment on attachment 8914528 [details] [diff] [review]
Remove race on TypeString() char buffers

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

::: js/src/vm/TypeInference.cpp
@@ -131,5 @@
>          return NonObjectTypeString(type);
>  
> -    static char bufs[4][40];
> -    static unsigned which = 0;
> -    which = (which + 1) & 3;

Wow, that's pretty bad!  I sure hope this doesn't get used more than four times in single expression.

I don't really like having to allocate stack buffers in advance though.

How about allocating and returning a UniqueChars from these?  As I understand it, the rules around temporary objects mean that the returned temporary will live long enough (until the end of the complete expression containing it) so you won't have to do anything fancy.  You may need to call .get() to get the char pointer out.
Attachment #8914528 - Flags: review?(jcoppeard)
(Assignee)

Comment 9

a year ago
Aw man, I was being lazy. That is indeed what I was considering doing, but was scared off from figuring out the rules around temporary objects. But ok:

"All temporary objects are destroyed as the last step in evaluating the full-expression that (lexically) contains the point where they were created, and if multiple temporary objects were created, they are destroyed in the order opposite to the order of creation. This is true even if that evaluation ends in throwing an exception."

That sounds fine, then. And way cleaner and simpler than the stupid char buf[100]'s that I scattered everywhere.
(Assignee)

Comment 10

a year ago
It's a little ugly, because it creates a bunch of heap churn -- most things are returning a static constant string, but UniqueChars doesn't have a non-owning variant, so I guess I'll have to strdup them.
(Assignee)

Comment 11

a year ago
Created attachment 8915252 [details] [diff] [review]
Remove race on TypeString() char buffers

The get() is unfortunate, but I guess it's fine.
Attachment #8915252 - Flags: review?(jcoppeard)
(Assignee)

Updated

a year ago
Attachment #8914528 - Attachment is obsolete: true
Comment on attachment 8915252 [details] [diff] [review]
Remove race on TypeString() char buffers

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

Great, thanks for making the updates.
Attachment #8915252 - Flags: review?(jcoppeard) → review+

Comment 13

a year ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d4353f7c8b7
Remove race on TypeString() char buffers, r=jonco

Comment 15

a year ago
Pushed by sfink@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1470a3142fad
Remove race on TypeString() char buffers, r=jonco
https://hg.mozilla.org/mozilla-central/rev/1470a3142fad
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.