Skip to content
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

cbprintf: Package size calculation is using best case alignment #33208

Closed
nordic-krch opened this issue Mar 10, 2021 · 3 comments
Closed

cbprintf: Package size calculation is using best case alignment #33208

nordic-krch opened this issue Mar 10, 2021 · 3 comments
Labels
bug The issue is a bug, or the PR is fixing a bug

Comments

@nordic-krch
Copy link
Contributor

Describe the bug
cbprintf_package(NULL, 0, ..) supposes to return required space needed to pack string with arguments. It is using NULL address for calculating alignment which is 8 and 16 bytes aligned. However, size depends on initial alignment of the buffer. As a result following code may fail:

int len = cbprintf_package(NULL, 0, "test %f", d);
uint8_t *buf = alloca(len); /* buffer not 8 bytes aligned */
len = cbprintf_package(buf, len, "test %f", d);
if (len == -ENOSPC) {
  LOG_ERR("no space");
} 

Expected behavior
There are few options:

  • add requirement that buffer must be aligned to 8 or 16 bytes (it is for long double which is rare, so it may be optional)
  • calculate worse case as if buffer was not aligned to 8 or 16 (but require that buffer is 4 bytes aligned)
  • when NULL is provided to indicate length calculation use len field to indicate alignment of the buffer that will be used.

Impact
Unexpected error due to too small buffer.

@nordic-krch nordic-krch added the bug The issue is a bug, or the PR is fixing a bug label Mar 10, 2021
@nordic-krch
Copy link
Contributor Author

@npitre @pabigot what's your view on this?

@pabigot
Copy link
Collaborator

pabigot commented Mar 10, 2021

Document that the buffer needs to be aligned and sized based on the requirements of the arguments, and that if not some format operations will fail due to space lost to enforce necessary alignment.

Or stop trying to do alignment in the packed version. I can see alternating %c with %llu being a problem (without looking at how that's actually handled).

@nordic-krch
Copy link
Contributor Author

Didn't notice earlier but it states in the api: The pointer must be aligned to a multiple of the largest element in the argument list

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug
Projects
None yet
Development

No branches or pull requests

2 participants