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

[Henri Verbeet]

On 4 September 2016 at 11:24, Marcus Meissner  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.
> @@ -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.

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

[Matteo Bruni]

2016-09-05 9:34 GMT+02:00 Henri Verbeet :
> On 4 September 2016 at 11:24, Marcus Meissner  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.

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

[Henri Verbeet]

On 6 September 2016 at 01:41, Matteo Bruni  wrote:
> 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.

Right, it checks the input buffer size, but not the output buffer
size. Still, you effectively calculate the length of the name in
"pos".