search

RfidResearchGroup / proxmark3

Iceman Fork - Proxmark3
http://www.icedev.se
GNU General Public License v3.0
3.62k stars 979 forks source link

Another fix to the Buffer SaveStates #2369

Closed jlitewski closed 1 month ago

jlitewski commented 2 months ago

I updated the tests for the Buffer SaveStates and noticed another issue with it, and this PR fixes that issue.

So with my galaxy sized brain (implied sarcasm, btw), in the save/restore code for 8-bit buffers, I forgot that not all numbers are divisible by 4, so Iceman fixed that flaw. In doing so, the outputted size of the buffer was bigger than the original size, causing issues when reconstructing data. I modified the buffer_savestate_t structs to have a const uint8_t padding which keeps track of any padding that is needed for restoring the buffer. Now, no matter what buffer goes in, it comes out correctly and is the right sized.

github-actions[bot] commented 2 months ago

You are welcome to add an entry to the CHANGELOG.md as well

iceman1001 commented 2 months ago

I don't think we wanna have four if statements for each iteration.

Instead of all this extra, why not just adjust the original length with the extra bytes?

jlitewski commented 2 months ago

I don't think we wanna have four if statements for each iteration.

Instead of all this extra, why not just adjust the original length with the extra bytes?

Because it's unknown what size the buffer is with the restore commands. The original design was that buffer_savestate_t would work with any buffers used with it, not just the Graph/Operation/Demod buffers. Adding the bytes to the length would mean whatever buffer that was originally passed into it would have to be either larger than the original length passed in or be reallocated for the extra bytes after creating a save state. The Graph/Operation/Demod buffers wouldn't have issues because they are allocated larger than what they use, but any other 8-bit buffer would have issues if they only allocated enough space to fit the data they are holding.

Even though this fix isn't the cleanest, it guarantees that it'll be the same length going in as it would be coming out. The testing command explicitly doesn't use the same buffer to test and compare the data that went in and came out to make sure it's the same, but both buffers are the same length because in actual use, you would pass the same buffer into both commands to save/restore that state. If it's larger on one side than the other, an overflow would happen unless you made sure that the buffer going in was always allocated to a multiple of 4.

And if there's a better way to loop through an array while bit shifting stuff around that doesn't require four if statements to break out of that loop early, I'll be happy to implement it since I also don't like this solution but couldn't come up with another solution that would also not break that guarantee.