Skip to content

Added option to enable safety checks#298

Open
stefano-zanotti-88 wants to merge 2 commits intocharlesnicholson:mainfrom
stefano-zanotti-88:safe-ub
Open

Added option to enable safety checks#298
stefano-zanotti-88 wants to merge 2 commits intocharlesnicholson:mainfrom
stefano-zanotti-88:safe-ub

Conversation

@stefano-zanotti-88
Copy link
Contributor

This adds various checks against UB or things in which NPF differs from the standard, and return an error (and an empty string) in that case, so that the caller can more easily identify and fix the underlying problem.

When using the sprintf-style functions, failed checks cause the output string to be empty, and an error to be returned.

When using the streaming (callback) function directly, the chars already outputted cannot be undone, but a special value -1 is passed to the callback, to signal the error. This lets the callback try some cleanup, if possible -- eg clear its buffer if it hasn't already been flushed, or clear the whole output, if the "target" of the callback allows that.
Instead of -1, we could also use EOF, but we need to include stdio.h for that
In the buffer-based callback proposed in #287, this could be extended there as well, by passing a negative integer as the length.
Also, an error is returned by the function, just like for the sprintf ones.

Right now, upon error, the printf function returns the negative value of the line where the error was detected, to diagnose the problem more easily. More stable codes could be used if needed.
The same value could also be given to the putc callback, instead of a fixed -1.
As long as nanoprintf.h does not grow over 32767 lines, the negative line number is fine even on systems with 16-bit int.

Note: this PR will need reworking if/when other fixes/PRs are accepted. In particular, right now some flags/specifiers are overwritten too early to allow for proper safety checks. They should be delayed until later, if that's not detrimental to code size or performance in the case when safety checks are disabled.

I've added a file with tests for all of the checks introduced by this PR. It'll have to be integrated with the existing tests somehow.

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.

This is a very interesting PR- I'd like to be able to diff the changes to nanoprintf.h more closely but since all of npf_parse_format_spec has been relocated, the GitHub diff view makes it very challenging to look at line-by-line changes.

Can you do it in-place with where npf_parse_format_spec was originally? nanoprintf has some users (e.g. satellite firmware) where they need to analyze each diff, and large structural changes like this make that very challenging.

Once it compiles, we can look at the size footprint change when enabled / disabled.

The tests should be easy enough to port to the existing doctest testing code, though.

@stefano-zanotti-88
Copy link
Contributor Author

I had to move it because of order-of-declaration issues.
It wasn't something fixable with a forward declaration, if I recall correctly.
I'll have a look at it

@stefano-zanotti-88
Copy link
Contributor Author

For this, I'll be waiting for #327 to be sorted out, since those fixes are important for some UB to be detected here.

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.

3 participants