Page MenuHomeFreeBSD

Use parse_integer to avoid sign extension.
ClosedPublic

Authored by jhb on Jun 3 2019, 11:31 PM.

Details

Summary

Coverity warned about gdb_write_mem sign extending the result of
parse_byte shifted left by 24 bits when generating a 32-bit memory
write value for MMIO. Simplify the code by using parse_integer
instead of unrolled parse_byte calls.

CID: 1401600

Test Plan
  • It compiles.
  • I tested this by reading/writing to the last 4 bytes of standard config space of the hostbridge at 0:0:0 via the MCFG window.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

jhb created this revision.Jun 3 2019, 11:31 PM
cem accepted this revision.Jun 3 2019, 11:35 PM

I'd cast the left shift instead (leaving the return type matching the type and range of the decoded value), but don't feel strongly about it. This works.

This revision is now accepted and ready to land.Jun 3 2019, 11:35 PM
cem added inline comments.Jun 3 2019, 11:36 PM
usr.sbin/bhyve/gdb.c
397 ↗(On Diff #58211)

Oh, I guess I'd say please use "unsigned" in new code, if you go this route.

markj accepted this revision as: markj.Jun 4 2019, 3:36 AM
emaste added a comment.Jun 4 2019, 1:28 PM

LGTM

usr.sbin/bhyve/gdb.c
397 ↗(On Diff #58211)

I would tend to agree.

Also, we have many examples of both static unsigned and static unsigned int. Probably good to document our preference in style.9.

cem added inline comments.Jun 4 2019, 1:58 PM
usr.sbin/bhyve/gdb.c
397 ↗(On Diff #58211)

I'd rather leave unsigned vs unsigned int unspecified in style(9), because I prefer the shorter form and I'm not sure that's the majority opinion ¯\_(ツ)_/¯.

jhb added a comment.Jun 4 2019, 4:13 PM
In D20508#442887, @cem wrote:

I'd cast the left shift instead (leaving the return type matching the type and range of the decoded value), but don't feel strongly about it. This works.

I find that less readable to add a cast for one of the 4 calls to this function in an expression. This route seems to avoid the need to do similar hacks when decoding other multi-byte values in the future as well.

jhb added inline comments.Jun 4 2019, 4:15 PM
usr.sbin/bhyve/gdb.c
397 ↗(On Diff #58211)

So bhyve itself is already split between u_int vs 'unsigned foo'. However, this file already uses both 'unsigned int' and 'unsigned char', so 'unsigned int' is probably most consistent within the file. Looking in sys/sys/*, a bare unsigned is generally unused.

cem added a comment.Jun 4 2019, 4:20 PM
In D20508#443091, @jhb wrote:

I find that less readable to add a cast for one of the 4 calls to this function in an expression. This route seems to avoid the need to do similar hacks when decoding other multi-byte values in the future as well.

We could go even further and avoid similar "u_int byte" hacks by making our LP64 archs ILP64. Or, uh, changing our compiler to be noncompliant and promote to unsigned integer, rather than signed integer. (Neither of these are serious suggestions.)

I still feel that the cast as-needed is clearer than returning uint for a byte parser, but still don't feel strongly about it.

jhb updated this revision to Diff 58251.Jun 4 2019, 10:29 PM
  • Use parse_integer instead of unrolled parse_byte.
  • Add byte swapping since that seemed to be needed in testing.
This revision now requires review to proceed.Jun 4 2019, 10:29 PM
cem added inline comments.Jun 4 2019, 10:31 PM
usr.sbin/bhyve/gdb.c
957–960 ↗(On Diff #58251)

Maybe just be32dec() and be16dec()?

jhb added inline comments.Jun 4 2019, 11:17 PM
usr.sbin/bhyve/gdb.c
957–960 ↗(On Diff #58251)

The data are not binary, but ASCII hex, so parse_integer is doing a loop of parse_byte with shifts, but it assumes it is parsing BE data (as GDB's protocol sends integer values in BE). As such, I can't use the *dec routines directly.

cem added inline comments.Jun 5 2019, 12:25 AM
usr.sbin/bhyve/gdb.c
957–960 ↗(On Diff #58251)

Ah, ok. sscanf(data, "%08lx", &ulong); be32toh() then? :) I'm just joking around, it's fine as is.

jhb retitled this revision from Change parse_byte to return u_int to avoid sign extension. to Use parse_integer to avoid sign extension..Jun 5 2019, 12:29 AM
jhb edited the summary of this revision. (Show Details)
jhb edited the test plan for this revision. (Show Details)
This revision was not accepted when it landed; it landed in state Needs Review.Jun 5 2019, 11:38 PM
This revision was automatically updated to reflect the committed changes.