fix negative slice index error in keymutex #84
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
cncf-cla: no
|
weird, why cla check failed ? |
|
@danielqsj can you please sign the CLA? See https://github.com/kubernetes/community/blob/master/CLA.md for instructions and trouble shooting steps |
|
@dims I signed the CLA for CNCF a long time ago. And also checked again. |
k8s-ci-robot
added
cncf-cla: yes
|
@dims oops, thank you so much. |
|
@danielqsj is there some kind of test we could add? |
There was a problem hiding this comment.
@danielqsj is there some kind of test we could add?
Maybe a good use-case for a fuzzer?
keymutex/hashed.go
Outdated
| return nil | ||
| } | ||
|
|
||
| func (km *hashedKeyMutex) hash(id string) int { | ||
| func (km *hashedKeyMutex) hash(id string) uint { |
There was a problem hiding this comment.
it's fine to cast from uint32 to uint, but I'm not sure why we're doing that.
There was a problem hiding this comment.
hi @apelisse
The issue mentions that on arm machine, the output of int(h.Sum32()) maybe negative, and cause problems in LockKey and UnlockKey due to this negative number will be used as the slice index.
But I'm not sure, whether convert to uint is safe here.
Otherwise, math.Abs could be another option for this case ?
There was a problem hiding this comment.
Yeah, sorry I made myself unclear. I meant, why don't you just return an uncasted uint32?
There was a problem hiding this comment.
aha, that's right. I miss the return type of h.Sum32(). Thanks.
Already updated, PTAL.
keymutex/hashed.go
Outdated
| } | ||
|
|
||
| // Releases the lock associated with the specified ID. | ||
| func (km *hashedKeyMutex) UnlockKey(id string) error { | ||
| km.mutexes[km.hash(id)%len(km.mutexes)].Unlock() | ||
| km.mutexes[km.hash(id)%uint(len(km.mutexes))].Unlock() |
There was a problem hiding this comment.
I also think it's fine to cast the return of len() (int) into uint since len is always returning a zero or positive value.
| } | ||
|
|
||
| // Releases the lock associated with the specified ID. | ||
| func (km *hashedKeyMutex) UnlockKey(id string) error { | ||
| km.mutexes[km.hash(id)%len(km.mutexes)].Unlock() | ||
| km.mutexes[km.hash(id)%uint32(len(km.mutexes))].Unlock() |
|
/lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: apelisse, danielqsj The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
k8s-ci-robot
added
the
lgtm
xref: kubernetes/kubernetes#73858
The keymutex's slice index could be negative which should not happened.
This PR aims to make the index always positive.
/assign @thockin @dims