expand_number: Tighten check of unit.
ClosedPublic
Actions

Authored by delphij on Jun 10 2023, 4:34 AM.

Details

Reviewers

pjd
pluknet
des
emaste
ngie

Commits

rG08300d849485: expand_number: Tighten check of unit.

Summary

The current code silently ignores characters after the unit as long
the unit themselves were recognized. This commit makes expand_number(3)
to fail with EINVAL if buf did not terminate after the unit character.

While I am there, also write a few test cases to validate the behavior.

MFC-after: 2 weeks

Test Plan

Run the test cases

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 52001
Build 48892: arc lint + arc unit

Event Timeline

delphij created this revision.Jun 10 2023, 4:34 AM

Herald added a subscriber: imp. · View Herald TranscriptJun 10 2023, 4:34 AM

delphij requested review of this revision.Jun 10 2023, 4:34 AM

Harbormaster completed remote builds in B51987: Diff 123052.Jun 10 2023, 4:34 AM

delphij added a reviewer: ngie.Jun 10 2023, 4:35 AM

emaste added inline comments.Jun 11 2023, 3:54 PM

lib/libutil/expand_number.c
78–81	Preexisting issue, but looks like we need to mention `b` in the man page. I'm also not sure if b should mean `shift = 0` or should be an optional ignored character (i.e., so that `1k` and `1kb` are treated identically)
91–92	it looks like this does not need a line break?
lib/libutil/tests/expand_number_test.c
49	further to my comment above I'd say this correctly tests existing behaviour, but it seems as if `1kb` ought to be an accepted value

delphij marked 3 inline comments as done.Jun 12 2023, 6:02 AM

delphij added inline comments.

lib/libutil/expand_number.c
78–81	Actually I wasn't very sure why we supported `b`, but that was a good point. In the manual page change I'll say that the usage of `b` is discouraged (because some other utilities treats `b` as "block" and the result may be confusing).