Page MenuHomeFreeBSD

Adds uuid validation to bhyve(8) arg parsing
ClosedPublic

Authored by me_jamesrm.com on Apr 30 2021, 5:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 26, 12:49 PM
Unknown Object (File)
Thu, Oct 24, 11:37 AM
Unknown Object (File)
Sat, Oct 19, 1:43 PM
Unknown Object (File)
Wed, Oct 16, 9:50 AM
Unknown Object (File)
Sat, Oct 12, 10:33 PM
Unknown Object (File)
Sat, Oct 12, 4:37 AM
Unknown Object (File)
Mon, Oct 7, 11:41 AM
Unknown Object (File)
Sun, Oct 6, 4:04 AM

Details

Summary

Uses libc's uuid to check that the uuid supplied
at the command line is valid. If not, then bhyve
will exit with an appropriate error message.

Test Plan

Run bhyve passing a valid UUID, for ex:

bhyve -U 7c27ce48-7f96-4c2e-9ab5-3ccdd8a2ef39

and it should display the info message

Run

bhyve -U invalid-uuid

and see bhyve fail with an error message

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 38938
Build 35827: arc lint + arc unit

Event Timeline

me_jamesrm.com added a subscriber: kp.

Style feedback thanks @kp

In head this code has changed a bit, and you'd rather want to check at the place that the associated config file is used (so that you'd catch invalid UUID strings from config files).

bcr added a subscriber: bcr.

OK from manpages.

The uuid is being correctly validated in smbiostbl.c:smbios_type1_initializer() so there's no need to do it again. An fprintf to stderr there indicatiing it is invalid is fine, and the assert on a return from smbios_build() can be changed into an exit(1).

The uuid is being correctly validated in smbiostbl.c:smbios_type1_initializer() so there's no need to do it again. An fprintf to stderr there indicatiing it is invalid is fine, and the assert on a return from smbios_build() can be changed into an exit(1).

Concur, other than the exit code should be 4, as exit(1) indicates "powered off" and that is not what happened in this case.

Thanks @grehan .

I see that doing the validation twice makes no sense. However I do
like the idea of validating inputs early on in the program execution. It's
also where the rest of the command line argument validation exists, so it seems
like the natural place for it?

So I have taken a slightly different approach to your suggestion, and
replaced guest_uuid_str with the actual guest_uuid.

Thanks for the feedback @jhb ,

you'd rather want to check at the place that the associated config file is used

I am still new to bhyve, and haven't looked at the config file code yet,
but if the config file had an invalid UUID I'd expect a different error message than the
one shown for an invalid command line argument? (from a UX perspective)

I am a pretty new contributor, but wanted to share my opinion - if it's not the
right approach, then I am happy to rework as requested.

Ok, after additional feedback from @rgrimes I have reworked the patch. Thanks for the feedback.

@jhb I've looked a little more into the config stuff and had a couple of questions:

  1. Where should this UUID be in the config data?
  2. Where should it be validated?
rgrimes requested changes to this revision.May 1 2021, 11:22 PM
rgrimes added inline comments.
usr.sbin/bhyve/smbiostbl.c
604 ↗(On Diff #88472)

This should remain a return (-1); And the assert in bhyverun.c should be changed to an exit(4);

This revision now requires changes to proceed.May 1 2021, 11:22 PM
me_jamesrm.com marked an inline comment as done.

Cleanup

usr.sbin/bhyve/bhyverun.c
1394

Hmm, I wonder if we should also exit(4) here as well? and in all the other assert(error == 0) locations?

usr.sbin/bhyve/bhyverun.c
1394

Save that for another review :)

usr.sbin/bhyve/bhyverun.c
1392

Per style(9) when there is only 1 statement in a block you do not enclose it in { }, so this could become just:

if (error)
        exit(4);
me_jamesrm.com marked an inline comment as not done.

style(9)

This revision is now accepted and ready to land.May 2 2021, 12:31 PM
rpokala added inline comments.
usr.sbin/bhyve/bhyverun.c
1392

For the record, style(9) was amended several years ago to allow braces around single-line statements:

Space after keywords (if, while, for, return, switch).  Two styles of
braces (‘{’ and ‘}’) are allowed for single line statements.  Either they
are used for all single statements, or they are used only where needed
for clarity.  Usage within a function should be consistent.  Forever
loops are done with for's, not while's.

See https://reviews.freebsd.org/V3 / https://cgit.freebsd.org/src/commit/?id=abb14e5deda3

With examples like Apple's "goto fail" bug (https://www.imperialviolet.org/2014/02/22/applebug.html) I'm surprised style(9) only allows, and doesn't recommend or mandate braces around single-line statements.

With examples like Apple's "goto fail" bug (https://www.imperialviolet.org/2014/02/22/applebug.html) I'm surprised style(9) only allows, and doesn't recommend or mandate braces around single-line statements.

Style(9) reflects the actual style in the tree. It tries to balance readability and safety. Too many lines with just } on them makes code harder to read. Plus studies on the topic are inconclusive as to bug reduction. Finally, the braces style was inherited from Bell Labs with the 7th edition code, some of which survives to today... still, time marches on and with larger screens an extra brace isn't as big a deal as it was years ago, which is why it's allowed but not mandatory.

Great work James all who have provided review!

Towards the goal of consistency in the error messages, perhaps consider changing "Invalid UUID provided" to simply "Invalid UUID" or "invalid UUID configuration" to follow the example of other error messages? Perhaps the maintainers have opinions on this.

Update's error message as per the suggestion from @editor_callfortesting.org

This revision now requires review to proceed.May 7 2021, 8:15 AM
This revision is now accepted and ready to land.May 7 2021, 8:17 AM
usr.sbin/bhyve/bhyverun.c
1390
usr.sbin/bhyve/smbiostbl.c
602 ↗(On Diff #88778)

Direct use of fprintf to stderr isn't used in bhyve since a uart on stdio can put the tty into raw mode. There are wrapper macros (EPRINTLN or the like) that are used in bhyve instead.

This revision now requires review to proceed.Apr 16 2022, 3:54 PM

@jhb apologies.. I only just realised this had outstanding feedback :-o

I have made the changes you requested.

I don't see any manual page change. Did I miss something?

jhb added inline comments.
usr.sbin/bhyve/bhyverun.c
1390
This revision is now accepted and ready to land.Apr 21 2022, 5:27 PM

I don't see any manual page change. Did I miss something?

There's no manpage change needed. It's just adding an error message and always exiting instead of an assert that only exits for a debug build.

In D30050#793504, @jhb wrote:

I don't see any manual page change. Did I miss something?

There's no manpage change needed. It's just adding an error message and always exiting instead of an assert that only exits for a debug build.

Doesn't explain why the manpages group reviewer.

Doesn't explain why the manpages group reviewer.

It is simple to explain: some older revisions touched some of the monitored path in the herald transcript and triggered manpages. You can use the history compare button on the Phabricator UI to figure that out as well.

This revision was automatically updated to reflect the committed changes.