Potential bug(s) with leftover processing

[slifty]

I'm currently in the process of mapping out the processing logic and I'm trying to understand the current logic around partial packet processing / think there might be a bug lurking.

I wanted to run it by you in case I should open a patch.

It's related to this code:

    // If we ended on a partial packet last
    // time, finish that packet first.
    if (remainder > 0) {
      if (len < remainder) {
        this.leftover.set(buffer.subarray(offset, offset + len), this.ptr);
        return 1; // still have an incomplete packet
      }

In particular, I'm not sure of a few things here:

1. Why is the conditional if (len < remainder) as opposed to if (len + remainder < PACKET_LEN)

It seems to me that the logic as written could result in a situation where the remainder was, say, 150, and the new data had a length of 140. The result would be 150 + 140 = 290 which is well above PACKET_LEN (188) and could very well be a new packet (but is being skipped).

2. Why isn't this.ptr updated? My understanding is that this.ptr is supposed to indicate how much data is in this.leftover. When we add data to the leftover and return without processing, but without updating this.ptr that data would be overwritten the next time process() is invoked, right?

3. this.ptr is also potentially not updated later in this conditional.

Hopefully this makes sense; please forgive me if I'm just misunderstanding the point of this.ptr / len / remainder

[gliese1337]

Why is the conditional if (len < remainder) as opposed to if (len + remainder < PACKET_LEN)

ptr records how far along along we are in a buffer, and should range between 0 and 188. And the top of the process method, it will point to the end of data in the leftover buffer, which corresponds to how far along we are in a packet (because leftovers should always be aligned at the start of a packet).

'len' is the total number of bytes available to read right now.

remainder is how many bytes are needed to complete a packet, when we have a partial packet left over from a previous read.

So, if remainder is greater than 0, we had a partial packet, and we need to get the remainder of that packet to finish processing it before we start a new packet.

If len is less than remainder, then we cannot get a complete packet, so we just update the leftover with however many additional bytes we have... and then, yes, this.ptr should have been updated to reflect that. That's a bug!

It seems to me that the logic as written could result in a situation where the remainder was, say, 150, and the new data had a length of 140. The result would be 150 + 140 = 290 which is well above PACKET_LEN (188) and could very well be a new packet (but is being skipped).

If the remainder is 150 then we must have 38 leftover bytes. If you then get a buffer of 140 bytes, the you should end up with a total of 178 leftover bytes, and no demuxed packets (except you won't, because it looks like there is in fact a bug there!)

If this.ptr is 150 and you get a buffer of 140 bytes, then remainder should be 38, len will be greater than remainder, we will copy exactly 38 bytes into the leftover buffer, demux that packet, and then increment the offset into the new buffer by 38 bytes and continue with processing remaining packets. There will only be 102 bytes left, so we'll copy those bytes into the leftover buffer (after setting this.ptr appropriately) and return, so that partial packet can be handled on a subsequent call.

[gliese1337]

Just pushed a fix for that, along with some additional comments that should help make the logic clearer: 71e4497

[slifty]

If len is less than remainder, then we cannot get a complete packet, so we just update the leftover with however many additional bytes we have.

Ah HA! That makes tons of sense (I had remainder backwards!) -- thank you so much for taking the time to explain and for the patch.