linuxtrack icon indicating copy to clipboard operation
linuxtrack copied to clipboard

Initialization of buffer in hashing.h is wrong, causing crash when extracting TrackIR firmware

Open Vantskruv opened this issue 1 year ago • 11 comments

In the constructor of FashHash in hashing.h, buffer is reserved, but not allocated. Hence: buffer.reserve(length); should be replaced by: buffer.resize(length);

This fixed my problem when extracting firmware for the TrackIR, causing the application to crash and fail the extraction. With the above fix, it works.

Vantskruv avatar Sep 28 '23 16:09 Vantskruv

Created a pull-request to fix this problem. https://github.com/uglyDwarf/linuxtrack/pull/201

I am not sure if should close this topic as of yet ...

Vantskruv avatar Sep 29 '23 07:09 Vantskruv

Nice fix.

exuvo avatar Oct 28 '23 19:10 exuvo

In the constructor of FashHash in hashing.h, buffer is reserved, but not allocated. Hence: buffer.reserve(length); should be replaced by: buffer.resize(length);

This fixed my problem when extracting firmware for the TrackIR, causing the application to crash and fail the extraction. With the above fix, it works.

Hello, I was going through the code and I don't see how the original code with reserve would cause a crash (as it does allocate enough memory, that is initialized immediately after the reserve). Do you by any chance remember where exactly that crash occur?

Kind regards,

Michal

uglyDwarf avatar Apr 21 '24 20:04 uglyDwarf

I agree code looks like it should work either way but i think this fixed the firmware extraction crash for me too. Maybe the problem is elsewhere and this just happens to move things around.

exuvo avatar Apr 27 '24 12:04 exuvo

I do not see any allocation of the buffer, therefore it crashes. Hence, reserve does not allocate the array.

Vantskruv avatar Apr 28 '24 05:04 Vantskruv

Sorry, I was wrong about this. It seems reserve does allocate memory!

Though, I think the difference is that when doing reserve, it only allocates memory, but does not size the array. The vector is still 0 size, and accessing the array out of bounds causes the crash. So you need call push_back or insert to add elements to the array.

Using resize(), allocates the array, and sets the size of the array. So you can access all the elements in the array. Point me if I am wrong.

Vantskruv avatar Apr 28 '24 05:04 Vantskruv

Well then i am "pointing you" as you are wrong. Vector [] does not do bounds checking. Only if you use .at. Both reserve and resize allocate memory but resize also initializes the elements and updates the size variable. Elements can be accessed with both as long as you handle that with reserve they are not initialized to a known value and vectors size variable is unmodified.

exuvo avatar Apr 28 '24 13:04 exuvo

Then I am as confused as you are. As the change I made fixed the problem. :stuck_out_tongue_closed_eyes:

Vantskruv avatar Apr 28 '24 13:04 Vantskruv

A code change in unrelated sections of a program can move around memory placement, so there probably is a use after free, reference to old stack variable or a out of bounds bug somewhere that became non-crashing with this change. Valgrind should be able catch it if you want to give it a try.

exuvo avatar Apr 28 '24 13:04 exuvo

According to documentation, calling reserve only allocates memory (increasing its capacity size), it does not increase the vector size. In this case, you are accessing the vector at indexes higher than its vector size. This is called undefined behaviour according to the documentation: Source: https://cplusplus.com/reference/vector/vector/operator[]/

Vantskruv avatar Apr 28 '24 13:04 Vantskruv

According to documentation, calling reserve only allocates memory (increasing its capacity size), it does not increase the vector size. In this case, you are accessing the vector at indexes higher than its vector size. This is called undefined behaviour according to the documentation: Source: https://cplusplus.com/reference/vector/vector/operator[]/

Good thinking! It didn't occur to me... Anyway I changed the code to be more C++ like (avoiding these cludges altogether), so this shouldn't cause issues anymore... Kind regards,

Michal

uglyDwarf avatar May 01 '24 19:05 uglyDwarf