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

Unused parameter in WithTryPrivate causing client to send incorrect version byte in header. #2125

Merged
merged 2 commits into from
Dec 10, 2024

Conversation

victornor
Copy link
Contributor

Closes #2122
As described in the issue, the MQTTNet.Client sends incorrect version in the connect header, causing the server to respond with an unsupported protocol exception.

Digging into the encoding code, it turns out the issue is rooted in the EncodeConnectPacket method in MqttV3PacketFormatter.

If TryPrivate is true (default), a bitwise OR operation is applied to the protocol version (I don't know why this is done).

var protocolVersion = 3;
if (packet.TryPrivate)
{
    protocolVersion |= 0x80;
}

bufferWriter.WriteByte((byte)protocolVersion);

This results in the connect header containing an incorrect version:
(hex): 10 df 04 00 06 4d 51 49 73 64 70 (83) 82 00 0f 00
Instead of the expected
(hex): 10 df 04 00 06 4d 51 49 73 64 70 (03) 82 00 0f 00

After fixing the unused parameter in WithTryPrivate and using .WithTryPrivate(false), the connection is successful and no longer throws an UnsupportedProtocolVersion exception.

@chkr1011
Copy link
Collaborator

chkr1011 commented Dec 9, 2024

@victornor to my Suprise this feature seems to be broken since a while and only worked when using protocol version 3.1 (and not with 3.1.1). I added a bit of code to match the implementation in mosquitto.

Please let me know what you think.

Copy link
Contributor Author

@victornor victornor 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. TryPrivate didn't seem implemented to me, so defaulting to true had me a bit confused.

@victornor
Copy link
Contributor Author

@dotnet-policy-service agree

@chkr1011 chkr1011 merged commit 981ea02 into dotnet:master Dec 10, 2024
1 check passed
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.

MQTTNet sends incorrect protocol version (83)
2 participants