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

Use "%.*s" when printing VERSION #215

Merged
merged 1 commit into from
Dec 7, 2020

Conversation

suve
Copy link
Contributor

@suve suve commented Dec 3, 2020

Sorry for the déjà vu. So, I submitted PR #213 and checked that my patch works. After re-writing the patch as suggested, I did not, however, attempt to re-build the app again... and what do you know, the new patch was wrong.

The patch from #213 replaced the %s conversion specifier with %*s. However, the %*s conversion specifier defines the field width, i.e. minimum number of characters to print. The proper solution is to use %.*s (note the dot), which defines the precision, i.e. the maximum number of printed characters.

Again, sorry for not verifying the earlier patch. This one works fine (the Fedora package on s390x builds and passes the test suite).

Commit 7bb2fef attempted to fix
the "missing NUL-terminator in VERSION" issue by replacing the "%s"
conversion specifier with "%*s". However, this fix itself was wrong,
as "%*s" specifies the field width, i.e. _minimum_ number of characters
to print. The proper solution is to use "%.*s", which specifies the
precision, i.e. the _maximum_ number of printed characters.
Copy link
Contributor

@asottile asottile left a comment

Choose a reason for hiding this comment

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

🤦 hah, and I had assumed you would just copy paste the code I gave you instead of rewriting it -- I was very careful to use %.*s in the suggestion I made 😆

@suve
Copy link
Contributor Author

suve commented Dec 3, 2020

I guess I wrote it by hand instead of copy-pasting since VERSION is used in two places, and the suggestion was only for one of them, so I went "ah, easy enough, just add the asterisk". 😅

@chriskuehl chriskuehl merged commit b945f3a into Yelp:master Dec 7, 2020
@chriskuehl
Copy link
Contributor

Thanks for the fix, will release this in 1.2.4 (will take a little bit, waiting on Travis to build it for all architectures now).

@chriskuehl
Copy link
Contributor

Released in v1.2.4: https://github.com/Yelp/dumb-init/releases/tag/v1.2.4

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