Page MenuHomeFreeBSD

sysctl_register_oid: fix duplicate oid after d3f96f661050
ClosedPublic

Authored by dougm on Sep 27 2022, 8:10 AM.
Tags
None
Referenced Files
Unknown Object (File)
Jan 10 2023, 2:24 AM
Unknown Object (File)
Jan 9 2023, 7:39 PM
Unknown Object (File)
Dec 15 2022, 8:01 PM
Subscribers

Details

Summary

sysctl_register_oid must check the uniqueness of any newly computed oid_number in sysctl_register_oid.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dougm requested review of this revision.Sep 27 2022, 8:10 AM
dougm created this revision.

It looks like your version will result in more sequential oid numbers. But I don't see how the old version could create a duplicate. Could you please explain?

sys/kern/kern_sysctl.c
497

I like this style better, and it's more searchable too.

It looks like your version will result in more sequential oid numbers. But I don't see how the old version could create a duplicate. Could you please explain?

After your change, when the first oid_number tried is found to be in use, you usually increment the oid_number and use it, assuming that it is not also in use. Before your change, when the first oid-number tried is found to be in use, the code usually incremented the old number and iterated, to check whether that number was in use, and kept doing so until it found a number not in use. So, you have introduced a bug, which I am trying to address, by making sure that the oid_number is not in use.

sys/kern/kern_sysctl.c
497

I chose to write this in a way that does not suggest that 'parent' matters, because it does not.

It looks like your version will result in more sequential oid numbers. But I don't see how the old version could create a duplicate. Could you please explain?

After your change, when the first oid_number tried is found to be in use, you usually increment the oid_number and use it, assuming that it is not also in use. Before your change, when the first oid-number tried is found to be in use, the code usually incremented the old number and iterated, to check whether that number was in use, and kept doing so until it found a number not in use. So, you have introduced a bug, which I am trying to address, by making sure that the oid_number is not in use.

Oh, I see. I missed a goto retry. If you move that out of the if block then that would also fix the problem.

sys/kern/kern_sysctl.c
497

Then what about RB_NEXT(sysctl_oid_list, , p)? I really don't like sysctl_oid_list_RB_NEXT, because when I search through my editor for a regex like \<sysctl_oid_list\> or \<RB_NEXT\>, it won't find it.

It looks like your version will result in more sequential oid numbers. But I don't see how the old version could create a duplicate. Could you please explain?

After your change, when the first oid_number tried is found to be in use, you usually increment the oid_number and use it, assuming that it is not also in use. Before your change, when the first oid-number tried is found to be in use, the code usually incremented the old number and iterated, to check whether that number was in use, and kept doing so until it found a number not in use. So, you have introduced a bug, which I am trying to address, by making sure that the oid_number is not in use.

Oh, I see. I missed a goto retry. If you move that out of the if block then that would also fix the problem.

It would fix the problem, but it would use RB_FIND again, when another tree search isn't really necessary.

Offer an RB_NEXT compromise.

Approved. And even though I probably won't MFC the original, please be sure to add an "MFC With" to your commit message, just in case.

This revision is now accepted and ready to land.Sep 27 2022, 5:08 PM

You may want to run a script to creating and destroying 2^32 single SYSCTL OIDs to verify the logic.