Bug in read_fifo()

[martin-vitek]

Hello, when some conditions are met (tag is on the limit of the reader range), then it is possible, that reader return rx_len as 0 in mfrc630_iso14443a_select(): https://github.com/iwanders/MFRC630/blob/5768981807e9f8b0eb2b4c2f00ff3a334c620472/mfrc630.c#L551

Then rx_len is used by https://github.com/iwanders/MFRC630/blob/5768981807e9f8b0eb2b4c2f00ff3a334c620472/mfrc630.c#L554

But mfrc630_read_fifo() can't handle zero len (underflow in for's condition) https://github.com/iwanders/MFRC630/blob/5768981807e9f8b0eb2b4c2f00ff3a334c620472/mfrc630.c#L79

I fixed it by checking for zero len in mfrc630_read_fifo(),

void mfrc630_read_fifo(uint8_t* rx, const uint16_t len)
{
    if (len == 0) return;

It's working, but I'm not sure, that this is the best solution.

[iwanders]

Hey @martin-vitek , thanks for posting this. Interesting find! So we end up spinning 65335 times on that for on line 79 in case this happens then?

The i is not used in the body of the for loop, should we rewrite it to the safer:

for (i=1; i < len ; i++) 

Though, technically we can just bail out earlier. I'm not sure what's more correct. Maybe the best approach is just to check the rx_length when it is retrieved?

https://github.com/iwanders/MFRC630/blob/5768981807e9f8b0eb2b4c2f00ff3a334c620472/mfrc630.c#L551

I'm not sure if any card ever needs to be able to perform a read of length zero. I somehow feel like it would be good to preserve the possibility for that... What do you think?

[martin-vitek]

Yes, and rx pointer is incremented somewhere out of memory.

I think, that both are correct. The function shouldn't be so easily breakable, so your proposed fix sounds good. And bailing out earlier is probably a good idea too, because it isn't probably necessary to continue, if rx_len is zero.

I'm not an expert at RFID, so I have no idea about a read of length zero.

[iwanders]

I'm not an expert at RFID, so I have no idea about a read of length zero.

Neither am I! >_<

I implemented the proprosed fix in #12, I did not add the bail out, because I think you can still read other cards if you are resolving a collision. This seemed like the most minimal fix that addresses the issue.

[iwanders]

I've merged the fix for the underflow. Going to leave the reading of 0 length in the function that does the collision resolution.

Thanks again for reporting this and testing the fix.