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

Fix for accessing string content when precision is 0. #253

Merged
merged 4 commits into from
Jan 11, 2024

Conversation

DeanoBurrito
Copy link
Contributor

Hey there,
First of all thanks for nanoprintf - it's been a great addition to a few of my projects.

I came across an issue (or rather unexpected behaviour) when using it in my kernel, specifically when formatting a string as %.*s and passing 0 as the precision. As expected this will write 0 characters, but the pointer for the string content is still de-referenced in the loop condition (to check for a null terminator). This is fine if the pointer for the string content is valid, but if the pointer is null it results in null deref.

It might sound silly wanting to write a string with a precision of 0 and null for the data, but my use case is printing human-readable debug data passed from a device driver (where strings are represented with a data pointer + length). In this case a driver might not want to provide anything so it sets the length to 0 and data pointer to null. I could (and probably should) check for this across kernel/driver boundaries, but I feel it'd be nice to have this in nanoprintf as well.

Thanks,
Dean.

@charlesnicholson
Copy link
Owner

Heya @DeanoBurrito, thanks for the pull request and I think your use case makes good sense. I'll take a look ASAP!

Copy link
Owner

@charlesnicholson charlesnicholson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a suggestion, also could I ask you to add a specific unit test case for this in tests/unit_vpprintf.cc please?

Alternatively I'm happy to do this in a new PR if you don't have time / appetite. If you choose to do it, though, consider adding yourself to the contributors file at the root as well!

nanoprintf.h Outdated Show resolved Hide resolved
@DeanoBurrito
Copy link
Contributor Author

Test case is added! Let me know if there's any changes you'd like.
I went with passing the precision of 0 in the format string directly (rather than as a vararg argument, like in my use case) in order to keep the test as simple as possible. Since handling star args is tested elsewhere and wasnt part of the issue.

@charlesnicholson
Copy link
Owner

Looks great, thanks!

@DeanoBurrito
Copy link
Contributor Author

@charlesnicholson Hey again, do you want something done about the failing tests? They're all triggered by -Werror=format-overflow reporting the input to %s is null.

@charlesnicholson
Copy link
Owner

I'd recommend adding something like

  #pragma GCC diagnostic ignored "-Wformat-overflow"

near the top, around line 15-16. That should shut gcc up; it's very helpful except for when we want to do unsafe things intentionally to test :)

@charlesnicholson
Copy link
Owner

Well shit, that didn't do much better. It looks like this is a new gcc-only diagnostic, so it might have to go in #else clause between lines 12-13, so it doesn't get compiled for clang?

@DeanoBurrito
Copy link
Contributor Author

hmm, I'll give that a try - thanks :)

@charlesnicholson charlesnicholson merged commit 9899706 into charlesnicholson:main Jan 11, 2024
21 checks passed
@DeanoBurrito
Copy link
Contributor Author

That appears to have done it, thanks for the support.

@charlesnicholson
Copy link
Owner

Third time's the charm, I guess :) Thanks for the work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants