Skip to content

Conversation

@Sonic-Amiga
Copy link

logicIndex is unsigned, so logicIndex < 0 was never true. Also, since we're dumping in reverse order, we need to to predecrement, not post-decrement

Buffer underrun caused dumping random corrupt garbage instead of captured data

…ture

logicIndex is unsigned, so logicIndex < 0 was never true. Also, since we're
dumping in reverse order, we need to to predecrement, not post-decrement

Buffer underrun caused dumping random corrupt garbage instead of captured data

Signed-off-by: Pavel Fedin <pavel_fedin@mail.ru>
@Sonic-Amiga
Copy link
Author

Hello! This bug took lots of time to find despite its sheer simplicity, i hope you're interested in the fix.

@gillham
Copy link
Owner

gillham commented Oct 23, 2023

Thanks for submitting this. I fixed a similar one in commit 739144d and meant to check the rest of the variables for a similar issue and didn't get there. I think I would rather just make it signed since the Mega buffer is 7KB at the most. Let me think about it as maybe I need to fix my other unsigned -> signed change if I want to resurrect the 'agla_megaram' branch which can get to 56KB buffer so would need it to be unsigned.

@gillham
Copy link
Owner

gillham commented Oct 23, 2023

I don't think moving the logicIndex-- is doing the right thing as it appears the data at index zero will not be returned.
With the if check like that when it is zero you are resetting the logicIndex before we use the zero value.
I think the if (logicIndex == 0) change is the only part we really need right?

@gillham
Copy link
Owner

gillham commented Oct 23, 2023

Nevermind, I guess it uses zero and the next time around the if is true. I will test this out locally this week hopefully.

@Sonic-Amiga
Copy link
Author

Sonic-Amiga commented Oct 23, 2023

Exactly. The sequence is: wrap, store, increment. "Wrap" means rolling over from 1024 to 0. So after the final loop iteration the index stays at (last_used + 1). 0 is not a problem at all; but after storing logic_buffer[1023] your counter will hold 1024. Pre-decrement does it all.

You may of course just change index to signed; it looked kinda ugly for me; but that's a matter of pure taste

@Sonic-Amiga
Copy link
Author

Hello! You disappeared.

@Sonic-Amiga
Copy link
Author

@gillham ???

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants