search

FolkComputer / folk

🎁 Physical computing system.
https://folk.computer
Apache License 2.0
77 stars 4 forks source link

Fix mem overflow in mailbox code - snprintf returns the number of characters that could have been written #120

Closed nmichaud closed 9 months ago

nmichaud commented 9 months ago

From the docs (https://cplusplus.com/reference/cstdio/snprintf/) -

"If the resulting string would be longer than n-1 characters, the remaining characters are discarded and not stored, but counted for the value returned by the function."

osnr commented 9 months ago

Thanks -- have you been running into this as an issue for anything specifically? We should definitely catch it somehow but I wonder if we should just abort the whole system if we have an overflow (there's a good chance the statements won't parse anyway if they've been truncated).

You also left in some changes to Commit and stepTime that are probably worth discussing/merging separately (I think the Commit change will interact weirdly with multiword keys?)

nmichaud commented 9 months ago

Thanks -- have you been running into this as an issue for anything specifically? We should definitely catch it somehow but I wonder if we should just abort the whole system if we have an overflow (there's a good chance the statements won't parse anyway if they've been truncated).

I haven't run into it yet - I don't have that big of a folk system yet :). I just noticed it when I was working on mailbox clearing. I agree that we should probably signal it some other way (maybe abort isn't necessary - I wonder if there's a way to grow the shared mailbox). I suspect that there won't be a parse issue - the overflowing statement will be truncated and likely might not match to anything downstream (unless it truncates in the middle of a pattern) - but I figure that is better than an overflow causing difficult to track bugs later on.

You also left in some changes to Commit and stepTime that are probably worth discussing/merging separately (I think the Commit change will interact weirdly with multiword keys?)

Oops - I wasn't careful when creating the PR. I've force-pushed a new version with the just the mailbox changes. I didn't realize multi-word keys were possible for Commit - I may have to rethink how to override the Commit scope.

osnr commented 9 months ago

Added warning log, tested, seems reasonable: 681C9E7A-73C8-47F6-AB22-DBF810568A70-1290-0000BC400B88E446