New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report possible race issues #2622

Closed
baigd opened this Issue Sep 22, 2015 · 1 comment

Comments

2 participants
@baigd

baigd commented Sep 22, 2015

Hi, Developers of crate/crate,

I am writing to report two race issues on use of ConcurrentHashMap. The issues are reported by our tool in an automatic way. Although manually confirmed, they would be a false positive, given we do not know the specification of the program. We would very appreciate if you could check below for details and confirm with us whether they are real problems. For more information, please refer to our website: http://sav.sutd.edu.sg/?page_id=2845

File:
"crate/crate/sql/src/main/java/io/crate/jobs/JobExecutionContext.java"
Location: Line (238,114/72/103)
Description:
The remove operation in line 238 and the read operation in line 269 on "subContexts" are guarded by the lock "subContexts". If the intention is to guarantee exclusive access on "subContexts", then the put operations in line 114 and line 72 on "subContexts" may need to be synchronized on the same lock as well. Otherwise, the synchronized blocks in line 237 and line 268 may not be necessary (for efficiency concerns) and the concurrency access on "subContexts" can be relied on the ConcurrentHashMap (subContexts) itself.

File:
"crate/crate/sql/src/main/java/io/crate/metadata/ReferenceInfos.java"
Location: Line (137/139/143/149, 168)
Description:
If the synchronized block from line 136 to line 151 is to guarantee the atomicity of the get-remove-put operations on "schemas", then the put operation in line 168 should be properly locked so as to avoid breaking the atomicity. Relying on the ConcurrentHashMap to ensure exclusive access is dangerous since ConcurrentHashMap has no guarantee of exclusive access.

@seut

This comment has been minimized.

Show comment
Hide comment
@seut

seut Sep 22, 2015

Member

Hey @baigd,

thanks a lot for reporting.

File:
"crate/crate/sql/src/main/java/io/crate/metadata/ReferenceInfos.java"
Location: Line (137/139/143/149, 168)
This seems to be a false positive because the variable "schemas" at line 168 is a local variable (given as a method argument) and not related to the instance variable with the same name.

File:
"crate/crate/sql/src/main/java/io/crate/jobs/JobExecutionContext.java"
Location: Line (238,114/72/103)
Analysis is somehow correct because using "subContexts" as lock to prevent concurrent remove-and-get-remaining-size operations is misleading and should use a dedicated lock or should be eliminated by using a atomic counter for the number of entries inside the map. This is now fixed by e710e36.

Btw. please add the commit hash the analysis has taken place in future so your detailed description (specially line numbers) would fit ;)

Member

seut commented Sep 22, 2015

Hey @baigd,

thanks a lot for reporting.

File:
"crate/crate/sql/src/main/java/io/crate/metadata/ReferenceInfos.java"
Location: Line (137/139/143/149, 168)
This seems to be a false positive because the variable "schemas" at line 168 is a local variable (given as a method argument) and not related to the instance variable with the same name.

File:
"crate/crate/sql/src/main/java/io/crate/jobs/JobExecutionContext.java"
Location: Line (238,114/72/103)
Analysis is somehow correct because using "subContexts" as lock to prevent concurrent remove-and-get-remaining-size operations is misleading and should use a dedicated lock or should be eliminated by using a atomic counter for the number of entries inside the map. This is now fixed by e710e36.

Btw. please add the commit hash the analysis has taken place in future so your detailed description (specially line numbers) would fit ;)

@seut seut closed this Sep 22, 2015

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