Bug 229641 - /usr/bin/printf (so also internal printf in sh) ignores width and precision in %b format
Summary: /usr/bin/printf (so also internal printf in sh) ignores width and precision i...
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 11.1-STABLE
Hardware: Any Any
: --- Affects Some People
Assignee: freebsd-bugs mailing list
URL:
Keywords: patch, regression
Depends on:
Blocks:
 
Reported: 2018-07-09 13:24 UTC by Rudolf Čejka
Modified: 2018-08-22 04:30 UTC (History)
2 users (show)

See Also:


Attachments
Possible fix (667 bytes, patch)
2018-07-09 21:51 UTC, Pedro F. Giffuni
no flags Details | Diff
Possible fix + copyright update (945 bytes, patch)
2018-07-09 22:34 UTC, Pedro F. Giffuni
no flags Details | Diff
Take 2 (2.29 KB, patch)
2018-07-14 21:57 UTC, Pedro F. Giffuni
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rudolf Čejka 2018-07-09 13:24:06 UTC
/usr/bin/printf (so also internal printf in sh) ignores width and precision in %b format. The change is since commit base r265706 (May 2014), where macro PF() with width and precision interpretation has been replaced by direct call to fputs(), so width and precision are ignored now.

Expected result, for example from bash:

bash$ printf "%8.2b" "a\nb\n"
      a

Bad result from /usr/bin/printf and from sh:
 
sh$ /usr/bin/printf "%8.2b" "a\nb\n"
a
b
 
sh$ printf "%8.2b" "a\nb\n"
a
b
Comment 1 Pedro F. Giffuni freebsd_committer 2018-07-09 21:51:33 UTC
Created attachment 195011 [details]
Possible fix

Fix submitted by Garrett D'Amore.
Comment 2 Pedro F. Giffuni freebsd_committer 2018-07-09 22:34:53 UTC
Created attachment 195012 [details]
Possible fix + copyright update
Comment 3 Jilles Tjoelker freebsd_committer 2018-07-11 21:52:31 UTC
This looks like a valid bug.

This change should have some tests. The printf utility already has tests so adding tests for this bug seems appropriate.

The copy of the format string is unnecessary as `start` is already a copy. Getting rid of it would also fix the bug that there is a memory leak if the allocation for bfmt succeeds but the allocation for p fails.

(On another note, `start` can be a rather large allocation on the stack. Ideally, no space would be reserved past the specifier such as b, c, s.)
Comment 4 Pedro F. Giffuni freebsd_committer 2018-07-14 21:57:51 UTC
Created attachment 195131 [details]
Take 2

Something like this (Not sure about the test).
Comment 5 Pedro F. Giffuni freebsd_committer 2018-07-30 18:39:31 UTC
(In reply to Pedro F. Giffuni from comment #4)
I have no idea what fails when I add the test:
___
kyua test -k /usr/tests/usr.bin/sed/Kyuafile
inplace_race_test:main  ->  passed  [0.222s]
legacy_test:main  ->  passed  [0.335s]
multi_test:main  ->  failed: 3 of 130 tests failed  [1.086s]
sed2_test:inplace_hardlink_src  ->  passed  [0.041s]
sed2_test:inplace_symlink_src  ->  passed  [0.022s]
sed_test:c2048  ->  passed  [0.020s]
sed_test:emptybackref  ->  passed  [0.025s]
sed_test:longlines  ->  passed  [0.026s]
sed_test:preserve_leading_ws_ia  ->  passed  [0.019s]
sed_test:rangeselection  ->  passed  [0.089s]

Results file id is usr_tests_usr.bin_sed.20180730-183642-566700
Results saved to /home/pfg/.kyua/store/results.usr_tests_usr.bin_sed.20180730-183642-566700.db
___

I just don't know my way around the testsuite. I am tempted to commit the fix and let someone else deal with the test.
Comment 6 Pedro F. Giffuni freebsd_committer 2018-08-07 22:57:51 UTC
For reference here is the replicated bug on Illumos:

https://www.illumos.org/issues/9646
Comment 7 commit-hook freebsd_committer 2018-08-07 23:04:16 UTC
A commit references this bug:

Author: pfg
Date: Tue Aug  7 23:03:51 UTC 2018
New revision: 337440
URL: https://svnweb.freebsd.org/changeset/base/337440

Log:
  Fix printf(1) ignores width and precision in %b format.

  The precision with behavior is "unspecified" by POSIX (as of 2018), but
  most implementations seem to have taken it to be treated the same as for
  "s"; applied after the unescaping.
  Adopt the same treatment on our printf.

  PR:	229641
  Submitted by:	Garrett D'Amore (illumos)

Changes:
  head/usr.bin/printf/printf.c
Comment 8 Pedro F. Giffuni freebsd_committer 2018-08-07 23:13:08 UTC
(In reply to Pedro F. Giffuni from comment #5)

While I'd love to have an ATF test, and I will continue playing with the testsuite at a later time, ... it does seem this is undefined behavior, so a test is not really mandatory (?).

For now, it does seem a good idea to "fix" the issue before the next release (12.0) so I commited the fix.
Comment 9 commit-hook freebsd_committer 2018-08-08 15:25:37 UTC
A commit references this bug:

Author: pfg
Date: Wed Aug  8 15:25:01 UTC 2018
New revision: 337458
URL: https://svnweb.freebsd.org/changeset/base/337458

Log:
  Fix printf(1) ignores width and precision in %b format.

  The precision with the conversion specifier b is specified by POSIX: see
  point 7 in the reference documentation.

  This corrects previous wrong log in r337440.

  Reference: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html

  PR:	229641
  Reported by:	Rudolf Cejka
  Submitted by:	Garrett D'Amore (illumos)
  MFC after:	1 week

Changes:
  head/usr.bin/printf/printf.c
Comment 10 Pedro F. Giffuni freebsd_committer 2018-08-08 15:29:09 UTC
(In reply to Pedro F. Giffuni from comment #8)

Now that its clear the behaviour is specified by POSIX (Thanks Rudolf for pointing it out), the tests are more important, as is MFCing the change.

Help with the test/ATF is welcome :-/.
Comment 11 commit-hook freebsd_committer 2018-08-11 11:13:44 UTC
A commit references this bug:

Author: jilles
Date: Sat Aug 11 11:13:35 UTC 2018
New revision: 337618
URL: https://svnweb.freebsd.org/changeset/base/337618

Log:
  printf: Fix \c in %b in printf builtin exiting the shell after r337458

  SVN r337458 erroneously partially reverted r265885.

  This is immediately visible when running the Kyua/ATF tests for
  usr.bin/printf, which actually test sh's printf builtin.

  PR:		229641

Changes:
  head/usr.bin/printf/printf.c
Comment 12 Jilles Tjoelker freebsd_committer 2018-08-11 14:37:11 UTC
With the fix from SVN r337618 and svn ps fbsd:nokeywords y usr.bin/printf/tests/regress.bwidth.out, the tests from the patch "Take 2" pass and look good to me.
Comment 13 commit-hook freebsd_committer 2018-08-13 21:54:55 UTC
A commit references this bug:

Author: jilles
Date: Mon Aug 13 21:54:28 UTC 2018
New revision: 337728
URL: https://svnweb.freebsd.org/changeset/base/337728

Log:
  printf: Add test for width and precision in %b format

  PR:		229641
  Submitted by:	pfg

Changes:
  head/usr.bin/printf/tests/Makefile
  head/usr.bin/printf/tests/regress.bwidth.out
  head/usr.bin/printf/tests/regress.sh
Comment 14 commit-hook freebsd_committer 2018-08-14 22:57:37 UTC
A commit references this bug:

Author: pfg
Date: Tue Aug 14 22:57:29 UTC 2018
New revision: 337823
URL: https://svnweb.freebsd.org/changeset/base/337823

Log:
  MFC r337458, r337618:
  Fix printf(1) ignores width and precision in %b format.

  The precision with the conversion specifier b is specified by POSIX: see
  point 7 in the reference documentation.

  Include fix from jilles@ to avoid breaking the testsuite.

  Reference: http://pubs.opengroup.org/onlinepubs/9699919799/utilities/printf.html

  PR:	229641
  Reported by:	Rudolf Cejka
  Submitted by:	Garrett D'Amore (illumos)

Changes:
_U  stable/11/
  stable/11/usr.bin/printf/printf.c
Comment 15 commit-hook freebsd_committer 2018-08-22 04:28:22 UTC
A commit references this bug:

Author: pfg
Date: Wed Aug 22 04:27:34 UTC 2018
New revision: 338181
URL: https://svnweb.freebsd.org/changeset/base/338181

Log:
  MFC r337728: (committed by jilles)
  printf: Add test for width and precision in %b format

  PR:  229641

Changes:
_U  stable/11/
  stable/11/usr.bin/printf/tests/Makefile
  stable/11/usr.bin/printf/tests/regress.bwidth.out
  stable/11/usr.bin/printf/tests/regress.sh
Comment 16 Pedro F. Giffuni freebsd_committer 2018-08-22 04:30:25 UTC
Committed in 12 and 11 (doesn't apply cleanly in 10 and I can't test at this time).

Thanks!