Page MenuHomeFreeBSD

Use parse_integer to avoid sign extension.
ClosedPublic

Authored by jhb on Jun 3 2019, 11:31 PM.
Tags
None
Referenced Files
Unknown Object (File)
Dec 23 2023, 7:11 PM
Unknown Object (File)
Dec 20 2023, 2:23 AM
Unknown Object (File)
Dec 13 2023, 4:00 AM
Unknown Object (File)
Nov 30 2023, 7:02 AM
Unknown Object (File)
Nov 29 2023, 5:58 PM
Unknown Object (File)
Nov 10 2023, 3:14 AM
Unknown Object (File)
Oct 27 2023, 1:26 AM
Unknown Object (File)
Sep 8 2023, 8:14 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 - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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
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.

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.

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 ¯\_(ツ)_/¯.

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.

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.

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.

  • 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
usr.sbin/bhyve/gdb.c
957–960 ↗(On Diff #58251)

Maybe just be32dec() and be16dec()?

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.

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.