[PATCH] More ldb/tdb tests.

[Stefan Metzmacher]

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>