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

🐛 [Bug]: memory: protect field access with lock to avoid possible data race #2360

Closed
3 tasks done
yanke-xu opened this issue Mar 9, 2023 · 4 comments · Fixed by #2368
Closed
3 tasks done

🐛 [Bug]: memory: protect field access with lock to avoid possible data race #2360

yanke-xu opened this issue Mar 9, 2023 · 4 comments · Fixed by #2368

Comments

@yanke-xu
Copy link
Contributor

yanke-xu commented Mar 9, 2023

Bug Description

Fixed inconsistency and also potential data race in memory/memory.go:
s.db is read/written 8 times in memory/memory.go; 7 out of 8 times it is protected by s.mux.RLock()/Lock(); 1 out of 8 times it is read without a Lock, which is in func Conn() on L142.
A data race may happen when Conn() and other func like Set() are called in parallel.
The fix is to protect the access to s.db and save the result to a local variable.

How to Reproduce

Steps to reproduce the behavior:
Nil

Expected Behavior

Data Race

Fiber Version

v2.42.0

Code Snippet (optional)

No response

Checklist:

  • I agree to follow Fiber's Code of Conduct.
  • I have checked for existing issues that describe my problem prior to opening this one.
  • I understand that improperly formatted bug reports may be closed without explanation.
@welcome
Copy link

welcome bot commented Mar 9, 2023

Thanks for opening your first issue here! 🎉 Be sure to follow the issue template! If you need help or want to chat with us, join us on Discord https://gofiber.io/discord

@ReneWerner87
Copy link
Member

ReneWerner87 commented Mar 12, 2023

@UtopiaGitHub can you help us with a pull request and also do the fix in the storage repository
https://github.com/gofiber/storage/blob/main/memory/memory.go#LL139C22-L139C22

@yanke-xu
Copy link
Contributor Author

yanke-xu commented Mar 12, 2023

Yes. I commit the pull request to fix the bug. Thanks for your condition.

@gaby
Copy link
Member

gaby commented Mar 13, 2023

@ReneWerner87 @UtopiaGitHub I made the comment in the PR too. But I think Conn() is suppose to return a pointer to the memory field. The other storage providers return a pointer.

ReneWerner87 pushed a commit that referenced this issue Mar 14, 2023
Update memory.go

The fix is to protect the access to s.db and save the result to a local variable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants