KEMBAR78
Fixed potential torn reads in EventCounters by gfoidl · Pull Request #22923 · dotnet/efcore · GitHub
Skip to content

Conversation

@gfoidl
Copy link
Member

@gfoidl gfoidl commented Oct 8, 2020

The backing fields for the counters are long, so there was potential torn read on 32-bit platforms.
Volatile.Read ensures that the value is read in a atomic way, so no torn reads can occur.

The backing fields for the counters are `long`, so there was potential torn read on 32-bit platforms.
`Volatile.Read` ensures that the value is read in a atomic way, so no torn reads can occur.
@roji
Copy link
Member

roji commented Oct 8, 2020

See ongoing conversation in npgsql/npgsql#3223 (comment)

@roji roji requested review from roji and removed request for roji October 8, 2020 14:04
@roji roji self-assigned this Oct 8, 2020
@gfoidl
Copy link
Member Author

gfoidl commented Oct 9, 2020

Updated to use Interlocked.Read.

Too eagerly with find & replace -- sorry.
Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

There's also a Volatile.Read and Volatile.Write in CalculateAndReset below - these should actually be a single Interlocked.Exchange operation (set to 0 and get the previous value out). Any chance you can do that as part of this PR?

@gfoidl
Copy link
Member Author

gfoidl commented Oct 9, 2020

Any chance you can do that as part of this PR?

Of course 😃 --> 2d39eb9

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@roji roji merged commit ffb51b6 into dotnet:main Oct 9, 2020
@gfoidl gfoidl deleted the patch-1 branch October 10, 2020 08:58
@ajcvickers
Copy link
Contributor

@dotnet/efteam Should we take this in 5.0?

@roji
Copy link
Member

roji commented Oct 22, 2020

I'm not sure this qualifies... It only affects 32-bit systems (really rare nowadays), and even there is really non-fatal. On the flip-side, the risk here is extremely low.

@ajcvickers
Copy link
Contributor

Let's discuss in triage.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants