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

[catloop] Always Bound Read from Control Duplex Pipe #836

Merged
merged 1 commit into from
Jul 10, 2023

Conversation

iyzhang
Copy link
Contributor

@iyzhang iyzhang commented Jul 7, 2023

This PR removes the ability to ever ready from the control duplex pipe on the accept with anything other than the size of MAGIC_CONNECT. Because each MAGIC_CONNECT represents a separate connection request, the code will crash if we ever read more than one at a time. Thus, this PR closes #777 by simply removing the ability to read anything from the control duplex pipe that is not a single MAGIC_CONNECT value.

@iyzhang iyzhang added the bug Something Isn't Working label Jul 7, 2023
@iyzhang iyzhang requested a review from ppenna July 7, 2023 22:46
@iyzhang iyzhang self-assigned this Jul 7, 2023
@iyzhang iyzhang requested a review from anandbonde July 7, 2023 22:46
@iyzhang iyzhang force-pushed the bugfix-catloop-always-bound branch from faf2c80 to 2b0dfef Compare July 7, 2023 23:37
@ppenna ppenna changed the title [catloop] Bug Fix: Always bound read from control duplex pipe [catloop] Always Bound Read from Control Duplex Pipe Jul 10, 2023
Copy link
Contributor

@ppenna ppenna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Didn't we fix this bug before this change? I noted that we were already issuing a bound pop with sizeof(MAGIC_CONNECT).

@iyzhang iyzhang force-pushed the bugfix-catloop-always-bound branch from 2b0dfef to 8f0f2f5 Compare July 10, 2023 15:39
@iyzhang
Copy link
Contributor Author

iyzhang commented Jul 10, 2023

We did. I just made it not optional.

@iyzhang iyzhang merged commit 6a04157 into dev Jul 10, 2023
11 checks passed
@iyzhang iyzhang deleted the bugfix-catloop-always-bound branch July 10, 2023 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something Isn't Working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[catloop] Resend connect request causes server not to recognize magic number
2 participants