On Mon, Jun 27, 2011 at 8:00 AM, Fam Zheng <famc...@gmail.com> wrote: > On Mon, Jun 27, 2011 at 12:54 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote: >> On Mon, Jun 27, 2011 at 4:48 AM, Fam Zheng <famc...@gmail.com> wrote: >>> Parse vmdk decriptor file and open mono flat image. >>> @@ -598,6 +600,154 @@ static int vmdk_open_vmdk4(BlockDriverState *bs, int >>> flags) >>> return ret; >>> } >>> >>> +/* find an option value out of descriptor file */ >>> +static int vmdk_parse_description(const char *desc, const char *opt_name, >>> + char *buf, int buf_size) >>> +{ >>> + char *opt_pos = strstr(desc, opt_name); >>> + int r; >>> + const char *end = desc + strlen(desc); >>> + >>> + if (!opt_pos) { >>> + return -1; >>> + } >>> + opt_pos += strlen(opt_name) + 2; >>> + if (opt_pos >= end) { >>> + return -1; >>> + } >>> + r = sscanf(opt_pos, "%[^\"]s", buf); >>> + return r <= 0; >>> +} >> >> This is still unsafe. Please see my comments on the previous version >> of this patch. > How about this: > > static int vmdk_parse_description(const char *desc, const char *opt_name, > char *buf, int buf_size) > { > char *opt_pos, *opt_end; > const char *end = desc + strlen(desc);
Already game over here because desc is not NUL-terminated. Either make desc NUL-terminated or add a desc_size argument. > > opt_pos = strstr(desc, opt_name); And again here. > if (!opt_pos) { > return -1; > } > /* Skip "=\"" following opt_name */ > opt_pos += strlen(opt_name) + 2; > if (opt_pos >= end) { > return -1; > } > opt_end = opt_pos; > while (opt_end < end && *opt_end != '"') { > opt_end++; > } > if (opt_end == end || buf_size < opt_end - opt_pos + 1) { > return -1; > } > strncpy(buf, opt_pos, opt_end - opt_pos); > buf[opt_end - opt_pos] = '\0'; cutils.c:pstrcpy() is easier to use than strncpy(), no need for the explicit NUL-termination. Stefan