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

[dv/dpi] TCP server enters infinite loop if ring buffer is full #24616

Open
duk-37 opened this issue Sep 21, 2024 · 2 comments · May be fixed by #24627
Open

[dv/dpi] TCP server enters infinite loop if ring buffer is full #24616

duk-37 opened this issue Sep 21, 2024 · 2 comments · May be fixed by #24627

Comments

@duk-37
Copy link

duk-37 commented Sep 21, 2024

Description

The rptr and wptr variables here are missing volatile, so GCC will optimize the while loop in tcp_server_put_byte to a branch and an infinite loop when compiling with -O2. as you can see in this godbolt link.

@rswarbrick
Copy link
Contributor

Oh dear! Thanks for the report and for analysing what exactly is going on here. If I understand correctly, the tcp_buffer_put_byte function expects some other thread to update rptr and, because it isn't volatile, we get the infinite loop you describe.

Presumably a trivial way to get this effect would be to pass buf is as volatile struct tcp_buf *buf or similar. It's "more volatile" than just the pointer variables (because we are allowing the outside world to change buf->buf as well), but the meaning might be more obvious (and doesn't require us to add annotations to the struct itself).

Would you be happy to file a PR with a fix?

@duk-37
Copy link
Author

duk-37 commented Sep 23, 2024

Oh dear! Thanks for the report and for analysing what exactly is going on here. If I understand correctly, the tcp_buffer_put_byte function expects some other thread to update rptr and, because it isn't volatile, we get the infinite loop you describe.

Yep!

Presumably a trivial way to get this effect would be to pass buf is as volatile struct tcp_buf *buf or similar. It's "more volatile" than just the pointer variables (because we are allowing the outside world to change buf->buf as well), but the meaning might be more obvious (and doesn't require us to add annotations to the struct itself).

Would you be happy to file a PR with a fix?

Sure, I can file one later today.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants