[PATCH] d3dxof: avoid some strcpy overflows (Coverity)

[Matteo Bruni]

2016-09-05 9:34 GMT+02:00 Henri Verbeet <hverbeet at gmail.com>:
> On 4 September 2016 at 11:24, Marcus Meissner <marcus at jet.franken.de> wrote:
>> @@ -532,8 +532,11 @@ static BOOL is_name(parse_buffer* buf)
>>    buf->rem_bytes -= pos;
>>
>>    TRACE("Found name %s\n", tmp);
>> +  if (strlen(tmp)+1 > sizeof(buf->value)) {
>> +    FIXME("name %s exceeds buffer length\n", tmp);
>> +    return FALSE;
>> +  }
>>    strcpy((char*)buf->value, tmp);
>> -
>>    return TRUE;
>>  }
> There are plenty of things wrong with d3dxof, but assuming the
> preceding code isn't too broken, I don't think this can happen. You
> should probably just get rid of the "tmp" buffer and use read_bytes()
> or something similar after calculating "pos" though. (The TRACE can
> use debugstr_an().) That probably applies to is_string() as well.

Hmm, unless there is something guaranteeing that the name can't be
longer than 100 bytes (and there might be, I haven't looked too
deeply) a check is still needed. Totally agree with the rest here and
below, though.

>> @@ -928,6 +937,10 @@ static BOOL parse_template_option_info(parse_buffer * buf)
>>      {
>>        if (get_TOKEN(buf) != TOKEN_NAME)
>>          return FALSE;
>> +      if (strlen((char*)buf->value)+1 > sizeof(cur_template->children[cur_template->nb_children])) {
>> +        FIXME("name %s too long for buffer\n", (char*)buf->value);
>> +        return FALSE;
>> +      }
>>        strcpy(cur_template->children[cur_template->nb_children], (char*)buf->value);
> Once you've calculated to number of bytes you need to copy anyway, you
> might as well just use memcpy(). I.e., strcpy() is almost always the
> wrong choice, for one reason or another.

FWIW that part also doesn't check if it's overrunning the array on the
other dimension i.e. that nb_children is less than MAX_CHILDREN. Not
that other parts of the code do check for it... The only time that's
checked in parsing.c is at the end of parse_object_parts() and that's
after the fact :/
I guess fixing that should be a separate patch anyway.