Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.Sign up
encoding/xml: Data race in xml/encoder due to getTypeInfo and shared slice #9796
Analysis described in http://stackoverflow.com/a/28374852/723947. For convenience, here's a copy of the analysis:
A user described a data race with the following trace:
I analyzed this. There's an implicit shared value due to the use of getTypeInfo() which is a type description of the struct. Other parts of the XML encoder assume that they can write and read type info, and this is unsafe, as the race detector is telling us.
The p.stack attribute that's reporting as the source of the data race originates from a part of the typeInfo shared value, where a slice of tinfo.parents gets injected on line 821.
That's ultimately where the sharing is happening with the potential for read and writing, because later on there are appends happening on the slice, and that can do mutation on the slice.
What should probably happen instead is that the finfo.parents should be sliced and capacity-restricted so that any potential append won't do a write on the shared value.
That is, line 821 could probably be be changed from:
to correct the issue.
Actually, my "fix" still looks dangerous. We really should be applying the capacity restriction on the initial assignment to s.stack on line 897.
Apologies for not reading this as carefully as I should have.
Doing the analysis from Go 1.4. Here's the point where the sharing happens:
Ultimately, the shared value flows into parentStack.stack, and there's both reads and writes going on. The read from parentStack.trim:
and the write from parentStack.push
Technically, I would guess that the append should logically be a no-op on the underlying array, because the parents that got split off should be the ones that are being appended back on, but that's something the data race detector can't possibly know.
@jeffallen Hmmm... something is strange and I am confused and need help in tracing this, before submitting the patch.
Let me show what I'm seeing. Here's a reduced program to demonstrate the issue:
As of commit bcadab9, which is the last time that src/runtime/slice.go has been touched according to git history, I can still see the data race:
However, when I get to master (ac90d9a), I'm not seeing the reporting of the data race anymore, as you note! But the data race is still logically there, right? Unless I'm misunderstanding something. Did something break in terms of error reporting the data race?
Unfortunately, I ran out of time tonight to binary search exactly at what point things erroneously succeed. I'll try to check it out when I have time during the weekend.
I think you are on to something here. Keep after it when you have a bit of
On Sat, Feb 7, 2015 at 5:06 AM, Danny Yoo email@example.com wrote:
Ok, I ran a
Ok, this sounds very related to the problem at hand. I don't quite understand the comment in the commit log though.
Let me try
I don't understand what's going on. This seems very tied to the garbage collector.
Can we notify @rsc about this to ask what the right behavior is supposed to be here?
Confirmed on go1.4.1, freebsd/amd64
Thanks for investigating this!
I've mailed https://go-review.googlesource.com/4370 with a fix for race detector.
With this change the race in your program is detected:
typedslicecopy is another write barrier that is not understood by racewalk. It seems quite complex to handle it in the compiler, so instead just instrument it in runtime. Update #9796 Change-Id: I0eb6abf3a2cd2491a338fab5f7da22f01bf7e89b Reviewed-on: https://go-review.googlesource.com/4370 Reviewed-by: Russ Cox <firstname.lastname@example.org>
This race was identified in #9796, but a sequence of fixes proposed in golang.org/cl/4152 were rolled into golang.org/cl/5910 which both fixed the race and modified the name space behavior. We rolled back the name space changes and lost the race fix. Fix the race separate from the name space changes, following the suggestion made by Roger Peppe in https://go-review.googlesource.com/#/c/4152/7/src/encoding/xml/marshal.go@897 Fixes #9796. Fixes #11885. Change-Id: Ib2b68982da83dee9e04db8b8465a8295259bba46 Reviewed-on: https://go-review.googlesource.com/12687 Reviewed-by: Ian Lance Taylor <email@example.com> Run-TryBot: Russ Cox <firstname.lastname@example.org>