[PATCH] More ldb/tdb tests.
Stefan Metzmacher
metze at samba.org
Sat Nov 7 08:15:04 UTC 2015
Hi Jeremy,
>> Here is the patch set. I've reviewed them, and made that clarifying
>> comment. Sadly we don't get the performance benefit until we get other
>> (also unsigned!) patches from matthieu's ldb-perfs branch.
>
> I'd like some changes here please.
>
> +static bool ldb_consume_element_data(uint8_t **pp, unsigned int *premaining)
> +{
> + unsigned int remaining = *premaining;
> + uint8_t *p = *pp;
> + uint32_t num_values = pull_uint32(p, 0);
> + uint32_t len;
> + int j;
> +
> + p += 4;
> + remaining -= 4;
> + for (j=0; j < num_values; j++) {
> + len = pull_uint32(p, 0);
> + if (len > remaining - 5) {
> + return false;
> + }
> + remaining -= len + 4 + 1;
> + p += len + 4 + 1;
> + }
> +
> + *premaining = remaining;
> + *pp = p;
> + return true;
> +}
>
> I'd prefer every pull from *pp be range checked against
> remaining for safety's sake. Also remember every addition
> from a pulled value also must be integer wrap checked.
> So make it something like:
>
> static bool ldb_consume_element_data(uint8_t **pp, unsigned int *premaining)
> {
> unsigned int remaining = *premaining;
> uint8_t *p = *pp;
> uint32_t num_values;
> uint32_t j;
>
> if (remaining < 4) {
> return false;
> }
>
> num_values = pull_uint32(p, 0);
> p += 4;
> remaining -= 4;
>
> for (j=0; j < num_values; j++) {
> uint32_t len;
>
> if (remaining < 4) {
> return false;
> }
> len = pull_uint32(p, 0);
> p += 4;
> remaining -= 4;
>
> if (len + 1 < len) {
> /* Integer wrap. */
> return false;
> }
> len += 1;
> if (remaining < len)
> return false;
> }
> remaining -= len;
> p += len;
> }
I don't like integeter wrap checks, they always completely confusing to me,
what about:
for (j=0; j < num_values; j++) {
uint32_t len;
if (remaining < 4) {
return false;
}
len = pull_uint32(p, 0);
p += 4;
remaining -= 4;
if (remaining < len) {
return false;
}
remaining -= len;
p += len;
if (remaining < 1) {
return false;
}
remaining -= 1;
p += 1;
}
subtraction checks are much easier to understand for me
compared to integer wrap checks.
> *premaining = remaining;
> *pp = p;
> return true;
> }
>
> Also the code changes inside ldb_unpack_data()
> require the same paranoia.
>
> These:
>
> + if (!keep) {
> + remaining -= len + 1;
> + p += len + 1;
>
> and these:
>
> + element->flags = 0;
> remaining -= len + 1;
> p += len + 1;
>
> require the range + wrap check paranoia tests.
Same here just handle len and 1 separately.
metze
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20151107/60262db3/signature.sig>
More information about the samba-technical
mailing list