View Change

2 comments:

• File cbtable.c:

  • Patch Set #5, 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.

• File fmap.c:

  • Patch Set #5, 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.

To view, visit change 30409. To unsubscribe, or for help writing mail filters, visit settings.