Eric Blake
2022-Nov-11 17:27 UTC
[Libguestfs] [libnbd PATCH] api: Generate flag values as unsigned
C says that performing bitwise math on ints can lead to corner-case
undefined values. When we document something as being usable as a
flag with bitwise-OR and similar, we should ensure the values are
unsigned, to avoid those corner-cases in C. Even our own codebase was
potentially falling foul of strict C rules, with code like
tests/errors-server-invalid-offset.c:
uint32_t strict;
strict = nbd_get_strict_mode (nbd) & ~LIBNBD_STRICT_BOUNDS;
In theory, this is an API change that might be detectable by someone
trying to probe which integer promotions occur on a given constant.
In practice, no one writes code that intentionally tries to exploit
whether a flag is signed or unsigned, and none of our values were
large enough to worry about sign-extension effects that occur when
flipping the sign bit of a signed value (we tend to target machines
with twos-complement signed values where the behavior is sane despite
being undefined by C), so this patch is therefore not an ABI change.
Generated code changes as follows:
| --- include/libnbd.h.bak 2022-11-11 11:15:54.929686924 -0600
| +++ include/libnbd.h 2022-11-11 11:15:56.652698919 -0600
| @@ -69,32 +69,32 @@
| #define LIBNBD_SIZE_MAXIMUM 2
| #define LIBNBD_SIZE_PAYLOAD 3
|
| -#define LIBNBD_CMD_FLAG_FUA 0x01
| -#define LIBNBD_CMD_FLAG_NO_HOLE 0x02
| ...
| -#define LIBNBD_ALLOW_TRANSPORT_VSOCK 0x04
| -#define LIBNBD_ALLOW_TRANSPORT_MASK 0x07
| +#define LIBNBD_CMD_FLAG_FUA 0x01U
| +#define LIBNBD_CMD_FLAG_NO_HOLE 0x02U
| ...
| +#define LIBNBD_ALLOW_TRANSPORT_VSOCK 0x04U
| +#define LIBNBD_ALLOW_TRANSPORT_MASK 0x07U
Thanks: Laszlo Ersek <lersek at redhat.com>
---
I'll push this along with the LIBNBD_STRICT_PAYLOAD series.
generator/C.ml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/generator/C.ml b/generator/C.ml
index cfa2964c..f9171996 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -437,11 +437,11 @@ let
List.iter (
fun (flag, i) ->
let flag = sprintf "LIBNBD_%s_%s" flag_prefix flag in
- pr "#define %-40s 0x%02x\n" flag i;
+ pr "#define %-40s 0x%02xU\n" flag i;
mask := !mask lor i
) flags;
let flag = sprintf "LIBNBD_%s_MASK" flag_prefix in
- pr "#define %-40s 0x%02x\n" flag !mask;
+ pr "#define %-40s 0x%02xU\n" flag !mask;
pr "\n"
) all_flags;
List.iter (
--
2.38.1
Richard W.M. Jones
2022-Nov-15 16:13 UTC
[Libguestfs] [libnbd PATCH] api: Generate flag values as unsigned
On Fri, Nov 11, 2022 at 11:27:51AM -0600, Eric Blake wrote:> C says that performing bitwise math on ints can lead to corner-case > undefined values. When we document something as being usable as a > flag with bitwise-OR and similar, we should ensure the values are > unsigned, to avoid those corner-cases in C. Even our own codebase was > potentially falling foul of strict C rules, with code like > tests/errors-server-invalid-offset.c: > > uint32_t strict; > strict = nbd_get_strict_mode (nbd) & ~LIBNBD_STRICT_BOUNDS; > > In theory, this is an API change that might be detectable by someone > trying to probe which integer promotions occur on a given constant. > In practice, no one writes code that intentionally tries to exploit > whether a flag is signed or unsigned, and none of our values were > large enough to worry about sign-extension effects that occur when > flipping the sign bit of a signed value (we tend to target machines > with twos-complement signed values where the behavior is sane despite > being undefined by C), so this patch is therefore not an ABI change. > > Generated code changes as follows: > > | --- include/libnbd.h.bak 2022-11-11 11:15:54.929686924 -0600 > | +++ include/libnbd.h 2022-11-11 11:15:56.652698919 -0600 > | @@ -69,32 +69,32 @@ > | #define LIBNBD_SIZE_MAXIMUM 2 > | #define LIBNBD_SIZE_PAYLOAD 3 > | > | -#define LIBNBD_CMD_FLAG_FUA 0x01 > | -#define LIBNBD_CMD_FLAG_NO_HOLE 0x02 > | ... > | -#define LIBNBD_ALLOW_TRANSPORT_VSOCK 0x04 > | -#define LIBNBD_ALLOW_TRANSPORT_MASK 0x07 > | +#define LIBNBD_CMD_FLAG_FUA 0x01U > | +#define LIBNBD_CMD_FLAG_NO_HOLE 0x02U > | ... > | +#define LIBNBD_ALLOW_TRANSPORT_VSOCK 0x04U > | +#define LIBNBD_ALLOW_TRANSPORT_MASK 0x07U > > Thanks: Laszlo Ersek <lersek at redhat.com> > --- > > I'll push this along with the LIBNBD_STRICT_PAYLOAD series. > > generator/C.ml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/generator/C.ml b/generator/C.ml > index cfa2964c..f9171996 100644 > --- a/generator/C.ml > +++ b/generator/C.ml > @@ -437,11 +437,11 @@ let > List.iter ( > fun (flag, i) -> > let flag = sprintf "LIBNBD_%s_%s" flag_prefix flag in > - pr "#define %-40s 0x%02x\n" flag i; > + pr "#define %-40s 0x%02xU\n" flag i; > mask := !mask lor i > ) flags; > let flag = sprintf "LIBNBD_%s_MASK" flag_prefix in > - pr "#define %-40s 0x%02x\n" flag !mask; > + pr "#define %-40s 0x%02xU\n" flag !mask; > pr "\n" > ) all_flags; > List.iter ( > --Looks good, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org