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 line reader to resolve buffer issues on TLS connections #888

Merged
merged 8 commits into from
Apr 30, 2024

Conversation

mtmk
Copy link
Contributor

@mtmk mtmk commented Apr 29, 2024

The StreamReader usage when reading the server INFO causes buffer issues on the stream, as it is also read in the reader loop. The StreamReader might consume or return the read bytes back, leading to problems when used on a TLS connection with .NET versions lower than 8. This fix removes the StreamReader usage to address these issues.

fixes nats-io/nats.net#586 (comment)

The StreamReader usage when reading the server INFO causes buffer
issues on the stream, as it is also read in the reader loop. The
StreamReader might consume or return the read bytes back, leading to
problems when used on a TLS connection with .NET versions lower than 8.
This fix removes the StreamReader usage to address these issues.
@mtmk mtmk requested a review from scottf April 29, 2024 20:39
@scottf scottf requested a review from caleblloyd April 29, 2024 22:08
{
if (foundCR)
{
buffer[read - 2] = (byte)'\r';
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the first byte is \r, read will be 1, and foundCR will be true. Walking this loop in my head.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea I think it should be -1 good catch 💯

src/NATS.Client/Connection.cs Outdated Show resolved Hide resolved
src/NATS.Client/Connection.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@scottf scottf merged commit 0fafd5b into main Apr 30, 2024
1 check passed
@scottf scottf deleted the fix-line-reader branch April 30, 2024 17:29
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 this pull request may close these issues.

NotSupportedException in Connection.publish getting BufferedStream.Position
4 participants