Hi Baptiste,
On Mon, Sep 12, 2016 at 10:50:04AM +0200, Baptiste wrote:
> It can be merged like this.
OK done then, thanks!
I noticed a few minor issues which will not prevent the code from
working so I preferred to merge the patches as-is so that your code
gets more exposure quickly.
The issues are :
- some incorrect tests which are a bit too strict, but some of
them already existed. For example :
/* query id */
if (reader + 2 >= bufend)
return DNS_RESP_INVALID;
dns_p->header.id = reader[0] * 256 + reader[1];
reader += 2;
The ">=" above should be ">" because strictly speaking you only need
to read two bytes after the ID. There's a rule of thumb to avoid these
and still stay safe, it's to consider that the pointer is allowed to
equal the end *after* being increased but not *before* reading. So
if you plan to read 2 bytes (reader[0] and reader[1]), you have to
ensure that reader + 2 <= bufend (hence error if it's > end).
So for example, if you plan to read a single byte, in general you'll
do this :
if (reader >= bufend)
return ERROR;
foo = *reader++;
But if you look closer, what you've done exactly this in fact this :
if (reader + 1 > bufend)
return ERROR;
foo = reader[0];
reader++;
It could make sense to have a macro to read 1- and 2- bytes since it
happens a lot of times, but it would also obscure the code and make
it less easy to review, so I'm not sure it's worth it. However what
you may possibly do is this :
#define MAY_READ_BYTES(pointer, end, count) ((pointer) + (count) <= (end))
Then the code becomes :
if (!MAY_READ_BYTES(reader, bufend, 1))
return ERROR;
foo = reader[0];
reader++;
if (!MAY_READ_BYTES(reader, bufend, 2))
return ERROR;
len = reader[0] * 256 + reader[1];
reader += 2;
if (!MAY_READ_BYTES(reader, bufend, len))
return ERROR;
memcpy(bar, reader, len);
reader += len;
Then it's more readable without making it unauditable.
>From what I'm seeing, most of these occurrences have no impact because
they're never on a packet boundary, there are always data after. But it
could possibly affect the very last record of a packet for example. At
least the construct is safe and makes it easy to review each of them.
The other minor point is the behaviour of chunk_strncat(), which doesn't
stop on a zero byte, hence which behaves differently from the true
strncat(). Given that it takes a char* in input, I think it still makes
sense to make it stop on \0 and always append the trailing \0 like the
true strncat. Then the last call doing this :
ret = chunk_strncat(&dns_trash, "\0", 1);
Becomes useless as if it was a true strncat.
As you can see for now it's not an issue but it's better to address these
before the code is misused.
Thanks,
Willy
