Skip to content
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

fix: use sum64 to avoid checkptr race bug #2645

Merged
merged 2 commits into from
Nov 3, 2022
Merged

Conversation

lvrach
Copy link
Member

@lvrach lvrach commented Nov 3, 2022

Description

This fix addresses a panic when running with --race flag:

fatal error: checkptr: pointer arithmetic result points to invalid allocation

The culprit is the following line, which is called murmur3.Sum64

k1 := *(*uint32)(unsafe.Pointer(p))

The usage of unsafe is being condemned in this issue and we should probably consider changing the library all together

Does it make sense to use int64 ?

Yes, both the pglock library and postgress supports this:

pg_advisory_lock(key bigint)

https://www.postgresql.org/docs/9.1/functions-admin.html

It also works with negative numbers

To avoid code like this being added in the future, I am considering enabling --race flag for all the tests, hoping the times are not going up.

Notion Ticket

https://www.notion.so/rudderstacks/murmur-race-fix-25aa4f37c58047faa1ff7baed24c0960

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@codecov
Copy link

codecov bot commented Nov 3, 2022

Codecov Report

Base: 43.90% // Head: 43.86% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (d8ddb6c) compared to base (3659e12).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2645      +/-   ##
==========================================
- Coverage   43.90%   43.86%   -0.05%     
==========================================
  Files         187      187              
  Lines       40038    40038              
==========================================
- Hits        17579    17561      -18     
- Misses      21363    21381      +18     
  Partials     1096     1096              
Impacted Files Coverage Δ
services/rsources/handler.go 69.72% <0.00%> (-1.39%) ⬇️
processor/processor.go 71.36% <0.00%> (-0.83%) ⬇️
router/router.go 74.38% <0.00%> (+0.17%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lvrach lvrach marked this pull request as ready for review November 3, 2022 09:03
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.

4 participants