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

broker sends last will message with wrong retain flag #222

Closed
codami opened this Issue Jul 21, 2016 · 6 comments

Comments

Projects
None yet
3 participants
@codami

codami commented Jul 21, 2016

I have a client that sets the retain flag in its last will message.
On unexpected disconnect however, the broker sends this will message with the retain flag not set.
After going through the log below I've found a strange retain flag value: 32.

See log:

1469098794: New client connected from ::1 as client1 (c1, k60).
1469098794: Sending CONNACK to client1 (0, 0)
1469098794: Received PUBLISH from client1 (d0, q1, r1, m1, 'TELE1/active', ... (1 bytes))
1469098794: Sending PUBACK to client1 (Mid: 1)
1469098794: Sending PUBLISH to bridge1.main (d0, q1, r1, m1, 'TELE1/active', ... (1 bytes))
1469098794: Received PUBACK from bridge1.main (Mid: 1)
1469098794: Socket error on client client1, disconnecting.

1469098794: Sending PUBLISH to bridge1.main (d0, q1, r32, m2, 'TELE1/active', ... (1 bytes))

1469098794: Received PUBACK from bridge1.main (Mid: 2)

@codami

This comment has been minimized.

Show comment
Hide comment
@codami

codami Jul 21, 2016

It looks like a set retain flag gets implicitly converted from int (0x20 =32) into bool at some point in the code.
The _mosquitto_send_real_publish function takes that as a 0/1 value even though underneath it might be 0x20:
packet->command = PUBLISH | ((dup&0x1)<<3) | (qos<<1) | retain;

codami commented Jul 21, 2016

It looks like a set retain flag gets implicitly converted from int (0x20 =32) into bool at some point in the code.
The _mosquitto_send_real_publish function takes that as a 0/1 value even though underneath it might be 0x20:
packet->command = PUBLISH | ((dup&0x1)<<3) | (qos<<1) | retain;

@karlp

This comment has been minimized.

Show comment
Hide comment
@karlp

karlp Jul 21, 2016

Contributor

is it actually sending any "wrong flags" though? or is it just a print artifact of an int being treated as a bool?

Contributor

karlp commented Jul 21, 2016

is it actually sending any "wrong flags" though? or is it just a print artifact of an int being treated as a bool?

@codami

This comment has been minimized.

Show comment
Hide comment
@codami

codami Jul 22, 2016

yes, it sends retain=0 instead of retain=1 when publishing last will to existing clients.

Internally the broker retains this message, so new clients will get it, but a bridge connected to this broker doesn't.

codami commented Jul 22, 2016

yes, it sends retain=0 instead of retain=1 when publishing last will to existing clients.

Internally the broker retains this message, so new clients will get it, but a bridge connected to this broker doesn't.

@ralight

This comment has been minimized.

Show comment
Hide comment
@ralight

ralight Jul 22, 2016

Contributor

I've not been able to reproduce this problem, what platform and version of mosquitto are you using?

Contributor

ralight commented Jul 22, 2016

I've not been able to reproduce this problem, what platform and version of mosquitto are you using?

@codami

This comment has been minimized.

Show comment
Hide comment
@codami

codami Jul 22, 2016

Windows 7 Professional SP1 64bit
with both 1.4.8 and 1.4.9 versions

codami commented Jul 22, 2016

Windows 7 Professional SP1 64bit
with both 1.4.8 and 1.4.9 versions

ralight added a commit that referenced this issue Jul 22, 2016

@ralight ralight added this to the Fixes-next milestone Jul 22, 2016

@ralight

This comment has been minimized.

Show comment
Hide comment
@ralight

ralight Jul 22, 2016

Contributor

Thanks very much, I've now fixed this in a slightly different way to your PR.

This is ultimately down to older versions of VS not supporting stdbool.h and using a char as a bool in that situation. 1.5 will require stdbool.h and drop support for VS older than 2013.

Contributor

ralight commented Jul 22, 2016

Thanks very much, I've now fixed this in a slightly different way to your PR.

This is ultimately down to older versions of VS not supporting stdbool.h and using a char as a bool in that situation. 1.5 will require stdbool.h and drop support for VS older than 2013.

@ralight ralight closed this Jul 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment