Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/30409 )
Change subject: Fix -Wsign-compare trouble ......................................................................
Patch Set 5:
(2 comments)
https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c File cbtable.c:
https://review.coreboot.org/c/flashrom/+/30409/5/cbtable.c@43 PS5, Line 43: image + size - 0x10
I think this parses as (image + size) - 0x10, right?
Yes, it does. So what is the problem?
image + (size - 0x10)
might produce an integer overflow (so it would matter that we changed the type to unsigned). But
(image + size) - 0x10
does not (unless `size` isn't the real size of `image`, or `size < 0x10`, but these potential issues existed before). So it seems regression free to me.
https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c File fmap.c:
https://review.coreboot.org/c/flashrom/+/30409/5/fmap.c@99 PS5, Line 99: (len - sizeof(struct fmap))
If len < sizeof(struct fmap), we could just do an early return -2, so there's no problem of overflow […]
Sure, but not part of this patch (the problem is in the subtraction not the comparison).
I tried to reason further about how to make the subtraction safe without an explicit `if` (i.e. by ensuring that we'd have a negative number on the right side). C rules can be very scary here: Even with
offset <= (off_t)len - sizeof(struct fmap)
We'd risk to compare unsigned numbers, AFAICT: First, the - would be evaluated, if both types (signed `off_t` and unsigned `size_t` of the sizeof()) have the same rank, `len` would be converted back to `size_t`, overflow and we'd compare `0 <=` to a large number. With the assumption that the rank of `off_t` is greater-equal to the rank of `size_t`, this would work:
offset <= (off_t)((off_t)len - sizeof(struct fmap))
The inner cast to handle `off_t` having the higher rank and the outer for the case that they have equal ranks. Obviously, an explicit `if` would be better ;)
Oh and in case of equal ranks and overflow, the cast to `off_t` is implemen- tation defined ^^ fun.
-
Nico Huber (Code Review)