-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
lib: fix ubsan errors in cbvprintf_package #77963
Conversation
Hello @cujomalainey, and thank you very much for your first pull request to the Zephyr project! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks correct to me, and that it should generate the same code after optimization despite the addition of the local variable. I knew assuming NULL == zero was UB, didn't know about the general restriction on doing math on a pointer known to be null.
When I ran |
Definitely limit the clang-format output to the relevant changed lines (that suggestion bot is fairly new, not sure if there's a clear paradigm for how to use it yet?). IMHO excessive reformatting hurts and doesn't help when digging through history, and we're on like our fifth or sixth iteration of coding style process. But I'm really not the right person to be pontificating about hygiene stuff and will let others make stronger suggestions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refresh +1. Officially I take no position on the separate formatting patch; unofficially I think it's a Sisyphean mess. But cbprintf isn't my domain and it's not like the result is ugly or anything (though hilariously I see at least one space where it and checkpatch can't agree).
compliance checks are on unions which clang format changed, im assuming that should be ignored then? |
What is eta on merge? This is completely blocking our fuzzing. |
Exactly why formatting checks must never be blocking, always advisory. Because there are always changes: new checks, policy changes, bug fixes, no one is ever on the same toolchain/software and/or config file version, etc. Been there, done that. |
compliance issues need to be fixed before this can be merged. |
@nashif the compliance issues were caused by clang-format is the correct procedure to override clang format? |
By sheer coincidence I just spotted some I have no idea whether this is by design or a checker bug. I saw this while looking for something else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix. I guess I’m the lone dissenter on formatting fixes here. Either way, the change looks good and should move forward.
@cujomalainey so we may move further with this PR can you undo the changes to the |
I believe the solution here really is to drop the second commit, as much as it pains me to say. I don't believe the clang-format checks are fully enabled and correct just yet. The first commit is the bug fix and should be merged. |
d7c90d0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the follow up and the fixing of the code! Sorry for the churn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refresh +1
Sorry I am going to do one more push to fix up the last of checks. |
It is undefined behaviour to shift / add offsets to a null pointer. Move to direct offset tracking to satisfy UBSAN. Simple translation of code: buf0 -> buf buf +=/++ -> offset +=/++ buf = -> buf+offset = Signed-off-by: Curtis Malainey <cujomalainey@chromium.org>
checks are clean, finally figured out how to format a patch directly. Thanks for the patience |
@nordic-krch @andyross @dkalowsk friendly ping, all checks are green. Can we push on this? |
Yes. Sorry fell off my radar when you updated it. @dcpleung can you also re-review? |
I don't know whether it's related and I don't have time to check (sorry) but I just remembered that sparse has been complaining about https://github.com/thesofproject/sof/actions/runs/11226456767/job/31207016541#step:7:282
|
not related to my change but if you can tell me how run that check I can fix it after |
Thanks! running sparse should be very easy now, see below. Fixing
See build sample script in thesofproject/sof@0061953 Off-topic sorry. |
@nordic-krch , can you please revisit this PR. |
Hi @cujomalainey! To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
Unfortunately, it breaks CI. Not sure why it was not run on this PR but there are failures: https://github.com/zephyrproject-rtos/zephyr/actions/runs/11288192171/job/31395611995 |
It is undefined behaviour to shift / add offsets to a null pointer.
Move to direct offset tracking to satisfy UBSAN.
Simple translation of code:
buf0 -> buf
buf +=/++ -> offset +=/++
buf = -> buf+offset