Skip to content

sys/fmt: disarm footgun#18162

Merged
miri64 merged 3 commits intoRIOT-OS:masterfrom
maribu:sys/fmt
Jun 5, 2022
Merged

sys/fmt: disarm footgun#18162
miri64 merged 3 commits intoRIOT-OS:masterfrom
maribu:sys/fmt

Conversation

@maribu
Copy link
Copy Markdown
Member

@maribu maribu commented Jun 2, 2022

Contribution description

Mixing print*() from fmt.h with puts() *printf() from stdio.h results in corrupted output even with only one thread printing. That's an obvious footgun. This fixes the issue.

Testing procedure

The first commit extends tests/fmt_print to detect output corruptions when mixing with stdio.h (this should fail on master). The second fixes the issue (so it should pass again after the second commit).

Issues/PRs references

Alternative to dropping the use of fmt in #18161

@maribu
tests/fmt_printf: spice up test
d98211f 
Also include interaction with stdio, as corrupting output on mixing
stdio and fmt is too much of a footgun.
@github-actions github-actions bot added Area: sys Area: System Area: tests Area: tests and testing framework labels Jun 2, 2022
@maribu maribu added the Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) label Jun 2, 2022
@maribu maribu mentioned this pull request Jun 2, 2022
Merged
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 2, 2022

@kaspar030 can you have a look at this?

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jun 2, 2022

I also sneaked in an uncrustify to make the CI happy.

@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 2, 2022
kaspar030
kaspar030 approved these changes Jun 2, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

This does now adds a dependency of fmt to newlib's stdio code (+200b for a print_str("foo\n") for an otherwise -DLOG_LEVEL=LOG_NONE build). But, no more broken output.

@kaspar030 kaspar030 enabled auto-merge June 2, 2022 19:41
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jun 2, 2022

Maybe should consider making picolibc the default. I was told @benpicco is suggesting just that for a few month now :)

maribu added 2 commits June 3, 2022 09:10
@maribu
sys/fmt: fix interaction with stdio
68dcf27 
Use fwrite instead of write to not bypass caching stdio. Other fmt's
print function are too much of a footgun to use.
@maribu
sys/fmt: uncrustify
ceeb28f
@maribu maribu added CI: run tests If set, CI server will run tests on hardware for the labeled PR CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 3, 2022
@maribu maribu disabled auto-merge June 3, 2022 07:10
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jun 3, 2022

I added special handling for native and some comments to explain why that is needed and squashed this right in.

It seems that consistent behavior among the different implementations is a bit more involved that I have hoped for, so lets run tests to be sure.

I disabled the auto merge in case someone wants to take a look at the changes.

@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jun 3, 2022

looks like the ESP32s from the pi fleet are unplugged / broken. But other than that this seems to be ready now

@maribu maribu removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Jun 4, 2022
@maribu maribu added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jun 4, 2022
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jun 5, 2022

All green now

@miri64 miri64 merged commit b77662d into RIOT-OS:master Jun 5, 2022
@maribu maribu deleted the sys/fmt branch June 5, 2022 19:21
@maribu
Copy link
Copy Markdown
Member Author

maribu commented Jun 5, 2022

Thx!

This was referenced Jun 9, 2022
Closed
Merged
@chrysn chrysn added this to the Release 2022.07 milestone Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kaspar030 kaspar030 kaspar030 approved these changes

@miri64 miri64 Awaiting requested review from miri64

@benpicco benpicco Awaiting requested review from benpicco

@fjmolinas fjmolinas Awaiting requested review from fjmolinas

@smlng smlng Awaiting requested review from smlng

@leandrolanzieri leandrolanzieri Awaiting requested review from leandrolanzieri leandrolanzieri is a code owner

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

@MichelRottleuthner MichelRottleuthner Awaiting requested review from MichelRottleuthner MichelRottleuthner is a code owner

Assignees

No one assigned

Labels

Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2022.07

Development

Successfully merging this pull request may close these issues.

4 participants

@maribu @miri64 @kaspar030 @chrysn