lib: fix ubsan errors in cbvprintf_package

[cujomalainey]

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

[github-actions]

Hello @cujomalainey, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

[andyross]

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.

[cujomalainey]

When I ran clang-format on the file i got back a lot more changes than just for my code, shall i drop another commit on top with all the changes to fix the file? Or fix underneath then put my changes on top?

[andyross]

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.

[andyross]

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).

[cujomalainey]

compliance checks are on unions which clang format changed, im assuming that should be ignored then?

[cujomalainey]

What is eta on merge? This is completely blocking our fuzzing.

[marc-hb]

we're on like our fifth or sixth iteration of coding style process.

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.

[nashif]

compliance issues need to be fixed before this can be merged.

[cujomalainey]

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?

[marc-hb]

By sheer coincidence I just spotted some ERROR : Test ClangFormat failed: message that is green anyway:
https://github.com/zephyrproject-rtos/zephyr/actions/runs/10814869996/job/30002370299?pr=78300

I have no idea whether this is by design or a checker bug. I saw this while looking for something else.

[cujomalainey]

@nashif I think the check is bad since changes you have submitted have the exact same formatting for unions 423264b

[dkalowsk]

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.

[mmahadevan108]

@cujomalainey so we may move further with this PR can you undo the changes to the unions that clang-format did.

[dkalowsk]

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.

[dkalowsk]

Thanks for the follow up and the fixing of the code! Sorry for the churn.

[andyross]

Refresh +1

[cujomalainey]

Sorry I am going to do one more push to fix up the last of checks.

[cujomalainey]

checks are clean, finally figured out how to format a patch directly. Thanks for the patience

[cujomalainey]

@nordic-krch @andyross @dkalowsk friendly ping, all checks are green. Can we push on this?

[dkalowsk]

@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?

[marc-hb]

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 cbvprintf since forever:

https://github.com/thesofproject/sof/actions/runs/11226456767/job/31207016541#step:7:282

/zep_workspace/zephyr/lib/os/printk.c: In function 'vsnprintk':
/zep_workspace/zephyr/lib/os/printk.c:246:19: warning: passing argument 1 of 'cbvprintf' from incompatible pointer type [-Wincompatible-pointer-types]
  246 |         cbvprintf(str_out, &ctx, fmt, ap);
      |                   ^~~~~~~
      |                   |
      |                   int (*)(int,  struct str_context *)
In file included from /zep_workspace/zephyr/include/zephyr/logging/log_msg.h:11,
                 from /zep_workspace/zephyr/include/zephyr/logging/log_core.h:9,
                 from /zep_workspace/zephyr/include/zephyr/logging/log.h:11,
                 from /zep_workspace/zephyr/lib/os/printk.c:22:
/zep_workspace/zephyr/include/zephyr/sys/cbprintf.h:744:27: note: expected 'cbprintf_cb' {aka 'int (*)(int,  void *)'} but argument is of type 'int (*)(int,  struct str_context *)'
  744 | int cbvprintf(cbprintf_cb out, void *ctx, const char *format, va_list ap)
      |               ~~~~~~~~~~~~^~~

[cujomalainey]

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 cbvprintf since forever:

https://github.com/thesofproject/sof/actions/runs/11226456767/job/31207016541#step:7:282

/zep_workspace/zephyr/lib/os/printk.c: In function 'vsnprintk':
/zep_workspace/zephyr/lib/os/printk.c:246:19: warning: passing argument 1 of 'cbvprintf' from incompatible pointer type [-Wincompatible-pointer-types]
  246 |         cbvprintf(str_out, &ctx, fmt, ap);
      |                   ^~~~~~~
      |                   |
      |                   int (*)(int,  struct str_context *)
In file included from /zep_workspace/zephyr/include/zephyr/logging/log_msg.h:11,
                 from /zep_workspace/zephyr/include/zephyr/logging/log_core.h:9,
                 from /zep_workspace/zephyr/include/zephyr/logging/log.h:11,
                 from /zep_workspace/zephyr/lib/os/printk.c:22:
/zep_workspace/zephyr/include/zephyr/sys/cbprintf.h:744:27: note: expected 'cbprintf_cb' {aka 'int (*)(int,  void *)'} but argument is of type 'int (*)(int,  struct str_context *)'
  744 | int cbvprintf(cbprintf_cb out, void *ctx, const char *format, va_list ap)
      |               ~~~~~~~~~~~~^~~

not related to my change but if you can tell me how run that check I can fix it after

[marc-hb]

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 void * warnings in pre-processor "meta-programming" on the other hand... I'm not so sure ;-)

• compile sparse and add to your PATH
• --cmake-args=-DZEPHYR_SCA_VARIANT=sparse --cmake-args=-DCONFIG_LOG_USE_VLA=n --cmake-args=-DCONFIG_MINIMAL_LIBC=y

See build sample script in thesofproject/sof@0061953

Off-topic sorry.

[mmahadevan108]

@nordic-krch , can you please revisit this PR.

[github-actions]

Hi @cujomalainey!
Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community!

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! 🪁

[nordic-krch]

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
The reason is that when buf !=NULL then ROUND_UP() will return different value depending on the alignment. offset always starts from 0, but input buffer may be aligned to 4 or 8. Previous approach was taking it into account and when function was called with buf==NULL then len was indicating what will be the alignment of the input buffer. Currently i don't see nice solution (except for using buf calculation when buf != NULL and offset calculation when it is NULL.