Re: [PATCH 03/10] reftable/record: handle overflows when decoding varints
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jan 20, 2025, 09:47 UTC
- Message-ID
- <CAOLa=ZQxp=tmmBwAV2OR9ODLGf_VHLxG_50-YwN7-s7+c6pmNQ@mail.gmail.com>
- In-Reply-To
- <20250116-b4-pks-reftable-sign-compare-v1-3-bd30e2ee96e7@pks.im>
Patrick Steinhardt <ps@pks.im> writes:
[snip]
Show 26 quoted lines
> diff --git a/reftable/record.c b/reftable/record.c
> index 04429d23fe..4e6541c307 100644
> --- a/reftable/record.c
> +++ b/reftable/record.c
> @@ -21,47 +21,40 @@ static void *reftable_record_data(struct reftable_record *rec);
>
> int get_var_int(uint64_t *dest, struct string_view *in)
> {
> - int ptr = 0;
> + const unsigned char *buf = in->buf;
> + unsigned char c;
> uint64_t val;
>
> - if (in->len == 0)
> + if (!in->len)
> return -1;
> - val = in->buf[ptr] & 0x7f;
> -
> - while (in->buf[ptr] & 0x80) {
> - ptr++;
> - if (ptr > in->len) {
> + c = *buf++;
> + val = c & 0x7f;
> +
> + while (c & 0x80) {
> + val += 1;I was at first confused, I understand that we add 1 to check if there is an overflow before adding the next section. But this actually modifies the value itself, but looking below at `put_var_int()`, we did value-- before storing each continuation byte. So during decoding.
Nit: it would be nice to explain that part a bit here with comments.
Show 38 quoted lines
> + if (!val || (val & (uint64_t)(~0ULL << (64 - 7))))
> + return -1; /* overflow */
> + if (buf >= in->buf + in->len)
> return -1;
> - }
> - val = (val + 1) << 7 | (uint64_t)(in->buf[ptr] & 0x7f);
> + c = *buf++;
> + val = (val << 7) | (c & 0x7f);
> }
>
> *dest = val;
> - return ptr + 1;
> + return buf - in->buf;
> }
>
> -int put_var_int(struct string_view *dest, uint64_t val)
> +int put_var_int(struct string_view *dest, uint64_t value)
> {
> - uint8_t buf[10] = { 0 };
> - int i = 9;
> - int n = 0;
> - buf[i] = (uint8_t)(val & 0x7f);
> - i--;
> - while (1) {
> - val >>= 7;
> - if (!val) {
> - break;
> - }
> - val--;
> - buf[i] = 0x80 | (uint8_t)(val & 0x7f);
> - i--;
> - }
> -
> - n = sizeof(buf) - i - 1;
> - if (dest->len < n)
> + unsigned char varint[10];
> + unsigned pos = sizeof(varint) - 1;
> + varint[pos] = value & 127;Nit: While the `get_var_int()` uses hexes, here we use ints. Would be nicer to use `0x7f` and so on and be consistent.
Show 19 quoted lines
> + while (value >>= 7) > + varint[--pos] = 128 | (--value & 127); > + if (dest->len < sizeof(varint) - pos) > return -1; > - memcpy(dest->buf, &buf[i + 1], n); > - return n; > + memcpy(dest->buf, varint + pos, sizeof(varint) - pos); > + return sizeof(varint) - pos; > } > > int reftable_is_block_type(uint8_t typ) > diff --git a/reftable/record.h b/reftable/record.h > index a24cb23bd4..721d6c949a 100644 > --- a/reftable/record.h > +++ b/reftable/record.h > @@ -34,6 +34,10 @@ static inline void string_view_consume(struct string_view *s, int n) > > /* utilities for de/encoding varints */ >
We should remove this, no?
Show 14 quoted lines
> +/* > + * Decode and encode a varint. Returns the number of bytes read/written, or a > + * negative value in case encoding/decoding the varint has failed. > + */ > int get_var_int(uint64_t *dest, struct string_view *in); > int put_var_int(struct string_view *dest, uint64_t val); > > diff --git a/t/unit-tests/t-reftable-record.c b/t/unit-tests/t-reftable-record.c > index 42bc64cec8..6d912b9c8f 100644 > --- a/t/unit-tests/t-reftable-record.c > +++ b/t/unit-tests/t-reftable-record.c > @@ -58,6 +58,22 @@ static void t_varint_roundtrip(void) > } > }
[snip]