ip_icmp ip_ctlprotox[] unbounded index

[thebugfixers_pm.me]

Summary

Add assertion ip_ctlprotox[] vs ip_p.

[glebius]

This event is impossible as u_char ip_p can't be greater than size of the array. You can add a compile time _Static_assert() and that change can be accepted.

[thebugfixers_pm.me]

Thanks for the review. Having read through the code again with a fresh pair of eyes, you are of course correct. Every day is an opportunity for learning as the old saying goes.

"flag for later" seems to be the most suitable course of action in the absence of a "delete diff" function.

[rscheff]

you can close/abandon the diff as the original submitter. or change it entirely to what Gleb suggests.

[tuexen]

I agree with Richard and Gleb.

[thebugfixers_pm.me]

Thank you to the reviewers for pointing this out.

[glebius]

Well, we really want to compare maximum value of ip_p to size of ip_ctlprotox[] array. We don't wont to compare 255 to 256 via preprocessor macro defines. Correct statement would be:

_Static_assert(nitems(ip_ctlprotox) - 1 == (typeof(((struct ip *)NULL)->ip_p))-1, "size of ip_ctlprotox[] doesn't match all IP protocols");

Also, some undocumented FreeBSD style so far is to put static asserts not in the middle of functions, but near corresponding variable declarations. This probably is a leftover from the times when static asserts were not part of the C language and they indeed used space and created symbols at compilation. So proper placement would be near the ip_ctlprotox array declaration. Since ip_protox is indexed the same way it also need a similar assertion.

[glebius]

To be fair, we got a lot of arrays indexed like that in FreeBSD and we don't have such static assertions, as this is so obvious. We got some static assertions for more complicated stuff. To be fair, I don't think that this change is a good one to be committed. Please find more bugs :)

[thebugfixers_pm.me]

@glebius

Thank you for _Static_assert(nitems(ip_ctlprotox) - 1 == (typeof(((struct ip *)NULL)->ip_p))-1, "size of ip_ctlprotox[] doesn't match all IP protocols");

As it happens that was roughly my first draft, but then I spent too much time thinking about whether I could do something smarter. I should have just submitted the original.

Overall the impression also got from a quick search through FreeBSD code is that _Static_assert overall is used sparingly in the codebase. So perhaps I'm better of calling it quits on this one.

Sadly despite the comments of the other reviewer, I don't appear to have the permissions to abandon it officially, the option doesn't appear on the right hand side for me, I only see:

Edit Revision
Update Diff
Download Raw Diff
Edit Related Revisions...
Edit Related Objects...
Mute Notifications
Remove Red Flag
Award Token

Please find more bugs :)

I'm keen to find bugs, trying to do my bit to contribute back to an OS I appreciate so much. :)

[tuexen]

In D57823#1326458, @thebugfixers_pm.me wrote:

@glebius

Thank you for _Static_assert(nitems(ip_ctlprotox) - 1 == (typeof(((struct ip *)NULL)->ip_p))-1, "size of ip_ctlprotox[] doesn't match all IP protocols");

As it happens that was roughly my first draft, but then I spent too much time thinking about whether I could do something smarter. I should have just submitted the original.

Overall the impression also got from a quick search through FreeBSD code is that _Static_assert overall is used sparingly in the codebase. So perhaps I'm better of calling it quits on this one.

Sadly despite the comments of the other reviewer, I don't appear to have the permissions to abandon it officially, the option doesn't appear on the right hand side for me, I only see:

Edit Revision
Update Diff
Download Raw Diff
Edit Related Revisions...
Edit Related Objects...
Mute Notifications
Remove Red Flag
Award Token

Please find more bugs :)

I'm keen to find bugs, trying to do my bit to contribute back to an OS I appreciate so much. :)

If you scroll down, you have a field with text "Add Action". If you click on it, there should be an option "Abandon Revision".