-
Notifications
You must be signed in to change notification settings - Fork 138
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ruleset: detect integer overflow of the ID and bail out #600
Conversation
ref: #475 |
@Cropi do you have any opinion on the integer overflow? |
Hi @muelli, |
ah, sounds like a valid approach. |
I think this can be merged for now. @muelli can you rebase PR branch? |
The check is semantically correct, because some IDs are reserved. Here, we exploit the fact that the LastID is defined to be max_int-2, so when we increment _id_next often enough, we will eventually reach the reserved LastID. If that ID is reached, we bail out to prevent further damage. The daemon logs the exception and keeps running. But it appears dysfunctional, i.e. does not notice new device let alone authorise any.
I've rebased the patch. |
@muelli @radosroka this commit and pull request broke the CI on multiple levels:
As such and with far-from-green CI, this pull request should not have been merged as-is in the first place. In the future, please let's keep CI green, fix red CI before anything else, and not merge things that are not green. Thank you! New pull request #628 provides fixes to all of the regression aspects and more, for a fully green CI. I'm happy to adjust details as needed, I believe it's the right way forward. Please let me know how you feel about it 🙏 CC @Cropi |
The check is semantically correct, because some IDs are reserved. Here, we exploit the fact that the LastID is defined to be max_int-2, so when we increment _id_next often enough, we will eventually reach the reserved LastID. If that ID is reached, we bail out to prevent further damage. We could have used the
__builtin_add_overflow
GCC extension but I don't know how compatible it is. Besides, we probably want to prevent the ID spilling into the reserved LastID anyway.The daemon logs the exception and keeps running. But it appears dysfunctional, i.e. does not notice new device let alone authorise any. That may or may not be the desired behaviour. But it serves as base for discussion and is probably more desired than the behaviour with the overflow.
To test the change, I have the following line in the assignID function: