Let's take a look at this code:
short bit = (payloadLength >> 0) & 1U; // get first bit
payloadLength &= 1UL << 0; // set first bit to 0
payloadLength = ntohl(payloadLength);
Although it's perfectly legal for you to order these statements this way, there's a risk that this won't work the way you want it to. Specifically, you probably want to decode the payload to use the host byte ordering before you start poking and prodding the bytes. Otherwise, if you send data from one system to another, there's a risk that you'll be reading the wrong bits back. But that's not too hard to fix - just move the last statement, which decodes payloadLength
, to the top, like this:
/* CAUTION: This still has errors! */
payloadLength = ntohl(payloadLength);
short bit = (payloadLength >> 0) & 1U; // get first bit
payloadLength &= 1UL << 0; // set first bit to 0
Next, there's a question about which bit you are trying to read. It looks like you are trying to read the least-significant bit of the number. If that's the case, you don't need to include any bitshifts, as bitshifting by zero positions doesn't have any effect. Let's remove those, giving this:
/* CAUTION: This still has errors! */
payloadLength = ntohl(payloadLength);
short bit = payloadLength & 1U; // get first bit
payloadLength &= 1UL; // set first bit to 0
Your code to extract the final bit of the number is correct. It'll mask out everything except the lowest bit, then store the value in the variable bit
. (Out of curiosity, is there any reason you're storing this as a short
? If you're looking for a boolean "do I need to reencode this?," you might want to just go with bool
and write something like
bool reencode = (payloadLength & 1U) != 0;
That's up to you to decide, though.)
However, your code to clear the last bit is incorrect. Right now, when you write
payloadLength &= 1UL; // set first bit to 0
you are actually doing the opposite of what you intended - you're clearing every bit except the first. That's because if you AND payloadLength
with a value, you're zeroing every bit in payloadLength
except for the bits that are equal to 1 in 1UL
. However, 1UL
only has a 1 bit in the last position. You probably meant to write something like
payloadLength &= ~1UL; // set first bit to 0
where the ~ operator flips the bits of your mask. Overall, this would give you the following:
payloadLength = ntohl(payloadLength);
short bit = payloadLength & 1U; // get first bit
payloadLength &= ~1UL; // set first bit to 0
I have one last question, though. By repurposing the lowest bit of the number this way, you are requiring that your payload length always be an even number, since you're harnessing the 1's bit of the payload length to encode whether to rekey. If you're okay with that, great! You don't need to do anything.
On the other hand, if that's an issue, you have a couple of options, which I'll leave to you to select from:
Instead of using the least-significant bit, use the most-significant bit. This will cause problems if you try to send payloads of size 231 or higher, though.
Use the least-significant bit, but have the upper 31 bits of the number represent the actual payload length. To extract the payload length, just shift everything over one position. This has the drawback of not supporting payloads of size 231 or greater, though.
Don't use the bits at all to encode this! Instead, send a payload length, then have the payload start with a header that tells you whether to rekey, etc. This uses more bytes per payload, but also gives you more flexibility in the future (what if you need to send other flags as well?)
Hope this helps!