Closed Bug 1161170 Opened 5 years ago Closed 4 years ago

TSan: data race gfx/cairo/cairo/src/cairo-freed-pool-private.h:80 _freed_pool_get

Categories

(Core :: Graphics, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: froydnj, Assigned: lsalzman)

References

(Blocks 1 open bug)

Details

(Whiteboard: [tsan][gfx-noted])

Attachments

(2 files, 2 obsolete files)

The attached logfile shows a thread/data race detected by TSan (ThreadSanitizer).

* Specific information about this bug

Cairo's freed_pool_t:

typedef struct {
    void *pool[MAX_FREED_POOL_SIZE];
    int top;
} freed_pool_t;

ensures that loads and stores to |pool| are done atomically, but it makes no such guarantees for accesses to |top|, which needs to be kept in sync with what slots of |pool| we are using.

Since we have to keep two pieces of information in sync, and we're doing this without locks, this seems rather sketchy.

* General information about TSan, data races, etc.

Typically, races reported by TSan are not false positives, but it is possible that the race is benign. Even in this case though, we should try to come up with a fix unless this would cause unacceptable performance issues. Also note that seemingly benign races can possibly be harmful (also depending on the compiler and the architecture) [1][2].

If the bug cannot be fixed, then this bug should be used to either make a compile-time annotation for blacklisting or add an entry to the runtime blacklist.

[1] http://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong
[2] _How to miscompile programs with "benign" data races_: https://www.usenix.org/legacy/events/hotpar11/tech/final_files/Boehm.pdf
Assignee: nobody → lsalzman
Hi Nathan and Lee,

I also ran into these data races while running ThreadSanitizer.
After experimenting with several fixes, I found that I just
need to declare |top| as an atomic type if C11 is available.

I don't have an account for https://bugs.freedesktop.org.
If you like this patch, please attach it to the upstream cairo
bug report for me. Thanks!
Attachment #8723270 - Flags: review?(lsalzman)
Attachment #8723270 - Flags: feedback?(nfroyd)
Comment on attachment 8723270 [details] [diff] [review]
Declare the |top| member of freed_pool_t with the C11 _Atomic keyword

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

This clearly fixes the bug; I'm not sure how acceptable it would be upstream.  I filed a bug:

https://bugs.freedesktop.org/show_bug.cgi?id=90318

and I think comment 1 in that bug suggests that if one could annotate the variable as the C++11 equivalent of memory_order_relaxed, everything would be OK.  I think _Atomic is actually a little too strong in that regard?
Attachment #8723270 - Flags: feedback?(nfroyd) → feedback+
Comment on attachment 8723270 [details] [diff] [review]
Declare the |top| member of freed_pool_t with the C11 _Atomic keyword

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

Nathan: thanks for the suggestion. I guess this patch is equivalent to
memory_order_seq_cst?

If I'm not mistaken, I cannot annotate the data type with the memory
order. So I will need to pass the memory order as an argument to the
C11 atomic_load_explicit() and atomic_store_explicit() functions,
which will necessitate a very intrusive patch.

If you think the upstream is likely to ask for that, then I can
prepare a new patch.
OK, here is the other approach, if the upstream prefers it.
While there is a theoretical race, the underlying pool of pointers are handled safely in a way that the consequence of the top variable being incorrect is harmless, since it just causes it to fall through to the full search. If you literally just picked a top value at random, the freed pool functions correctly the way it is written. Its value is advisory only.

We have pondered in the past just disabling this thing entirely, since it only turns up warnings while having nebulous benefits over just letting malloc handle it.

Adding more synchronization to it is only going to make it slower, at which point any advantages it has over just going through malloc rapidly deteriorate, since malloc already has optimizations of this sort.
Comment on attachment 8723302 [details] [diff] [review]
Use C11 atomics with memory_order_relaxed

May as well just take this patch, since I'm not sure we have any grand plans for merges with Cairo. If upstream does do something about it and we merge one day, we can always discard these changes.
Attachment #8723302 - Flags: review+
Comment on attachment 8723270 [details] [diff] [review]
Declare the |top| member of freed_pool_t with the C11 _Atomic keyword

R+'d the other version with memory_order_relaxed.
Attachment #8723270 - Attachment is obsolete: true
Attachment #8723270 - Flags: review?(lsalzman)
Comment on attachment 8723302 [details] [diff] [review]
Use C11 atomics with memory_order_relaxed

Lee: thanks for the review. I submitted a better version
of this patch to the Cairo upstream:
https://bugs.freedesktop.org/show_bug.cgi?id=90318#c5

Thanks to a patch that Nathan wrote last year, that patch
was easier to write than I expected.
Attachment #8723302 - Attachment is obsolete: true
Assignee: lsalzman → nobody
Looks like this was fixed upstream. Did that get pulled in to mozilla-central? Is this bug report still valid?
Flags: needinfo?(lsalzman)
Whiteboard: [tsan] → [tsan][gfx-noted]
(In reply to Anthony Hughes (:ashughes) [GFX][QA][Mentor] from comment #9)
> Looks like this was fixed upstream. Did that get pulled in to
> mozilla-central? Is this bug report still valid?

The submitter only made a patch upstream. We do not sync with upstream anymore except by cherry-picking patches on an as-needed basis, since we have diverged too far from upstream for merging to be trivial anymore.
Flags: needinfo?(lsalzman)
This is a straight backport of https://cgit.freedesktop.org/cairo/commit/?id=3538ac3e68f997d95d76865247717c90ae73630d
Assignee: nobody → lsalzman
Status: NEW → ASSIGNED
Attachment #8743277 - Flags: review?(jmuizelaar)
Attachment #8743277 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/mozilla-central/rev/9dd83314d24f
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.