Add critical section to protect s.currentRev #10975

lzhfromustc · 2019-08-04T23:56:31Z

Description
Among 14 read/write operations to s.currentRev, 10 of them are protected by s.revMu.RLock() or s.mu.Lock(), 3 of them are synchronized by using channel revc, but there is one read operation not protect by any Mutex, RWMutex or channel, which is in function store.Hash() at line 174 of file etcd/mvcc/kvstore.go:

etcd/mvcc/kvstore.go

Lines 167 to 175 in 9a2af73

    
           func (s *store) Hash() (hash uint32, revision int64, err error) { 
        
           	start := time.Now() 
        
           	s.b.ForceCommit() 
        
           	h, err := s.b.Hash(DefaultIgnores) 
        
           	hashSec.Observe(time.Since(start).Seconds()) 
        
           	return h, s.currentRev, err 
        
           }

What I did
Add s.revMu.RLock() and defer s.revMu.RUnlock() to protect s.currentRev

Why I did this
In the comment of s.revMu, it is said that s.revMu protects currentRev:

	// revMuLock protects currentRev and compactMainRev.
	// Locked at end of write txn and released after write txn unlock lock.
	// Locked before locking read txn and released after locking.
	revMu sync.RWMutex

Is it possible for this patch to introduce other bugs?
I think we only need to consider whether this patch will introduce double lock bug or not (double RLock can also produce deadlock bug if there is another goroutine).
I detected this bug by my static checker, so I can't provide runtime information to validate my patch. However, the buggy function is store.Hash(), and there is a test function that directly uses store.Hash() without using s.revMu.RLock(). So I think this patch is safe to use.

codecov-io · 2019-08-05T00:28:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@af19d01). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #10975   +/-   ##
=========================================
  Coverage          ?   64.02%           
=========================================
  Files             ?      401           
  Lines             ?    37542           
  Branches          ?        0           
=========================================
  Hits              ?    24037           
  Misses            ?    11877           
  Partials          ?     1628

Impacted Files	Coverage Δ
mvcc/kvstore.go	`85.57% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update af19d01...84316c6. Read the comment docs.

jingyih · 2019-08-05T21:02:07Z

This one is tricky. I did not find much description on this function's behavior. So not sure if / when we should lock revMu.

I assume the returned hash and revision should be consistent with each other. This means we need to lock revMu and read currentRev before using it to compute hash. But then this becomes basically identical to HashByRev().

@gyuho Is there documentation on this API? Is this API going to be deprecated in favor of HashByRev()?

gyuho · 2019-08-05T21:43:30Z

@jingyih Hash is used for functional testing. And HashByRev is used for peer-to-peer corruption check.

jingyih · 2019-08-05T23:22:29Z

I think we should get revision while generating the hash in the backend. The current way of returning store revision at the end of the function call (whether protected by revMu or not) is racy. Writes to store could happen after Hash() but before function returning.

@gyuho Any comment?

gyuho · 2019-08-06T05:55:49Z

@jingyih @lzhfromustc
Then acquiring read-lock at the beginning of Hash should be enough to block currentRev writes from *storeTxnWrite.

jingyih · 2019-08-07T03:01:03Z

If we want to block writes in Hash() function, we need to hold WRITE lock from the beginning of the function call, which is very expensive. Given that this is only used in functional test, maybe it is OK.

Alternatively we can open a db read transaction and compute hash and store revision from it - very similar to the approach in HashByRev().

gyuho · 2019-08-07T03:13:15Z

@jingyih Agree. In our functional tests, we just explicitly stop all clients, so there would be no race. Since it's meant for internal usage, let's keep it as it is now.

jingyih · 2019-08-07T18:04:42Z

mvcc/kvstore.go

@@ -171,6 +171,8 @@ func (s *store) Hash() (hash uint32, revision int64, err error) {
 	h, err := s.b.Hash(DefaultIgnores)

 	hashSec.Observe(time.Since(start).Seconds())
+	s.revMu.RLock()
+	defer s.revMu.RUnlock()


As discussed, adding lock here is not going to help much. For now, can we just add a TODO here as reminder?

I read your discussion but I am not familiar with these packages, so it's quite possible that my understanding is wrong.
Do you think the following is appropriate?

// TODO: hash and revision could be inconsistent, due to absence of s.revMu.Lock() // at the beginning of function, which is costly func (s *store) Hash() (hash uint32, revision int64, err error) {

Either adding s.revMu.RLock() or s.mu.Lock() can fix it. s.revMu.RLock() is likely better. There is a less costly way of fixing this, which is to get revision from the opened backend read transaction in s.b.Hash(). Probably just say:

"TODO: hash and revision could be inconsistent, one possible fix is to add s.revMu.RLock() at the beginning of function, which is costly".

jingyih · 2019-08-16T01:21:53Z

BTW could you also update the commit title to use format <package>:<description>

lzhfromustc · 2019-08-16T01:23:12Z

@jingyih Got it. I will commit the change and squash these commits shortly.

jingyih · 2019-08-30T05:46:00Z

@lzhfromustc friendly ping:)

lzhfromustc · 2019-08-31T03:12:15Z

@jingyih Sorry!!! I moved back to my university and completely forgot this.
Now I squashed my commit. It should be OK now.

jingyih · 2019-09-05T06:12:11Z

mvcc/kvstore.go

@@ -164,6 +164,7 @@ func (s *store) compactBarrier(ctx context.Context, ch chan struct{}) {
 	close(ch)
 }

+// TODO: hash and revision could be inconsistent, one possible fix is to add s.revMu.RLock() at the beginning of function, which is costly


https://travis-ci.com/etcd-io/etcd/jobs/229792898#L564

goword is suggesting to use 'Hash'.

jingyih · 2019-09-06T18:29:02Z

Sorry for all the back and forth. I misread earlier. goword checker actually demands the starting of the description to match the function name, which means it does not want the comment we are adding to start with word "TODO". We might want to move the comment inside the function.

Also, could you change the commit title to use format <package>:<description>.

Thanks for being patient:)

lzhfromustc · 2019-09-06T18:53:24Z

@jingyih No problem at all. Actually this informs me how format is checked in big open-source projects.
I think this time the format is OK, since the fmt checker has been received

jingyih · 2019-09-06T19:02:37Z

The commit title needs to be something like mvcc: add TODO to protect currentRev in Hash

…e of currentRev and suggest feasible fix

lzhfromustc · 2019-09-06T19:08:49Z

My bad! I mended it now

jingyih

LGTM.

Functional test failure is unrelated.

lzhfromustc mentioned this pull request Aug 5, 2019

Add critical section to protect a read operation s.compactMainRev #10976

Closed

gyuho requested a review from jingyih August 5, 2019 06:30

gyuho mentioned this pull request Aug 5, 2019

types: a potential double lock bug #10925

Closed

jingyih reviewed Aug 7, 2019

View reviewed changes

jingyih reviewed Sep 5, 2019

View reviewed changes

mvcc: add a TODO in (*store).Hash, to warn against potential data rac…

077dd74

…e of currentRev and suggest feasible fix

jingyih approved these changes Sep 6, 2019

View reviewed changes

jingyih merged commit 88d998b into etcd-io:master Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add critical section to protect s.currentRev #10975

Add critical section to protect s.currentRev #10975

lzhfromustc commented Aug 4, 2019

codecov-io commented Aug 5, 2019 •

edited

Loading

jingyih commented Aug 5, 2019

gyuho commented Aug 5, 2019

jingyih commented Aug 5, 2019

gyuho commented Aug 6, 2019

jingyih commented Aug 7, 2019 •

edited

Loading

gyuho commented Aug 7, 2019

jingyih Aug 7, 2019

lzhfromustc Aug 8, 2019

jingyih Aug 16, 2019

jingyih commented Aug 16, 2019

lzhfromustc commented Aug 16, 2019

jingyih commented Aug 30, 2019

lzhfromustc commented Aug 31, 2019

jingyih Sep 5, 2019

jingyih commented Sep 6, 2019

lzhfromustc commented Sep 6, 2019

jingyih commented Sep 6, 2019

lzhfromustc commented Sep 6, 2019

jingyih left a comment

	func (s *store) Hash() (hash uint32, revision int64, err error) {
	start := time.Now()

	s.b.ForceCommit()
	h, err := s.b.Hash(DefaultIgnores)

	hashSec.Observe(time.Since(start).Seconds())
	return h, s.currentRev, err
	}

Add critical section to protect s.currentRev #10975

Add critical section to protect s.currentRev #10975

Conversation

lzhfromustc commented Aug 4, 2019

codecov-io commented Aug 5, 2019 • edited Loading

Codecov Report

jingyih commented Aug 5, 2019

gyuho commented Aug 5, 2019

jingyih commented Aug 5, 2019

gyuho commented Aug 6, 2019

jingyih commented Aug 7, 2019 • edited Loading

gyuho commented Aug 7, 2019

jingyih Aug 7, 2019

Choose a reason for hiding this comment

lzhfromustc Aug 8, 2019

Choose a reason for hiding this comment

jingyih Aug 16, 2019

Choose a reason for hiding this comment

jingyih commented Aug 16, 2019

lzhfromustc commented Aug 16, 2019

jingyih commented Aug 30, 2019

lzhfromustc commented Aug 31, 2019

jingyih Sep 5, 2019

Choose a reason for hiding this comment

jingyih commented Sep 6, 2019

lzhfromustc commented Sep 6, 2019

jingyih commented Sep 6, 2019

lzhfromustc commented Sep 6, 2019

jingyih left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 5, 2019 •

edited

Loading

jingyih commented Aug 7, 2019 •

edited

Loading