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