| Message ID | 1488491046-2549-6-git-send-email-armbru@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 03/02/2017 03:43 PM, Markus Armbruster wrote: > sd_parse_uri() and sd_snapshot_goto() screw up error checking after > strtoul(), and truncate long tag names silently. Fix by replacing > those parts by new sd_parse_snapid_or_tag(), which checks more > carefully. At least we've fixed checkpatch to gripe at new uses of strtoul(), but yeah, we've got lots of existing poor usage. It is a very hard interface to use correctly, as evidenced by your cleanups here. > > sd_snapshot_delete() also parses snapshot IDs, but is currently too > broken for me to touch. Mark TODO. > > Two calls of strtol() without error checking remain in > parse_redundancy(). Mark them FIXME. > > More silent truncation of configuration strings remains elsewhere. > Not marked. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 55 insertions(+), 11 deletions(-) > Reviewed-by: Eric Blake <eblake@redhat.com>
Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben: > sd_parse_uri() and sd_snapshot_goto() screw up error checking after > strtoul(), and truncate long tag names silently. Fix by replacing > those parts by new sd_parse_snapid_or_tag(), which checks more > carefully. > > sd_snapshot_delete() also parses snapshot IDs, but is currently too > broken for me to touch. Mark TODO. > > Two calls of strtol() without error checking remain in > parse_redundancy(). Mark them FIXME. > > More silent truncation of configuration strings remains elsewhere. > Not marked. > > Signed-off-by: Markus Armbruster <armbru@redhat.com> > --- > block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++---------- > 1 file changed, 55 insertions(+), 11 deletions(-) > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index 5554f47..deb110e 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -914,6 +914,49 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp) > return fd; > } > > +/* > + * Parse numeric snapshot ID in @str > + * If @str can't be parsed as number, return false. > + * Else, if the number is zero or too large, set *@snapid to zero and > + * return true. > + * Else, set *@snapid to the number and return true. > + */ > +static bool sd_parse_snapid(const char *str, uint32_t *snapid) > +{ > + unsigned long ul; > + int ret; > + > + ret = qemu_strtoul(str, NULL, 10, &ul); > + if (ret == -ERANGE) { > + ul = ret = 0; > + } > + if (ret) { > + return false; > + } > + if (ul > UINT32_MAX) { > + ul = 0; > + } > + > + *snapid = ul; Redundant space. > + return true; > +} Looks good otherwise. Kevin
Kevin Wolf <kwolf@redhat.com> writes: > Am 02.03.2017 um 22:43 hat Markus Armbruster geschrieben: >> sd_parse_uri() and sd_snapshot_goto() screw up error checking after >> strtoul(), and truncate long tag names silently. Fix by replacing >> those parts by new sd_parse_snapid_or_tag(), which checks more >> carefully. >> >> sd_snapshot_delete() also parses snapshot IDs, but is currently too >> broken for me to touch. Mark TODO. >> >> Two calls of strtol() without error checking remain in >> parse_redundancy(). Mark them FIXME. >> >> More silent truncation of configuration strings remains elsewhere. >> Not marked. >> >> Signed-off-by: Markus Armbruster <armbru@redhat.com> >> --- >> block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++---------- >> 1 file changed, 55 insertions(+), 11 deletions(-) >> >> diff --git a/block/sheepdog.c b/block/sheepdog.c >> index 5554f47..deb110e 100644 >> --- a/block/sheepdog.c >> +++ b/block/sheepdog.c >> @@ -914,6 +914,49 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp) >> return fd; >> } >> >> +/* >> + * Parse numeric snapshot ID in @str >> + * If @str can't be parsed as number, return false. >> + * Else, if the number is zero or too large, set *@snapid to zero and >> + * return true. >> + * Else, set *@snapid to the number and return true. >> + */ >> +static bool sd_parse_snapid(const char *str, uint32_t *snapid) >> +{ >> + unsigned long ul; >> + int ret; >> + >> + ret = qemu_strtoul(str, NULL, 10, &ul); >> + if (ret == -ERANGE) { >> + ul = ret = 0; >> + } >> + if (ret) { >> + return false; >> + } >> + if (ul > UINT32_MAX) { >> + ul = 0; >> + } >> + >> + *snapid = ul; > > Redundant space. Will clean up. >> + return true; >> +} > > Looks good otherwise. > > Kevin Thanks!
diff --git a/block/sheepdog.c b/block/sheepdog.c index 5554f47..deb110e 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -914,6 +914,49 @@ static int get_sheep_fd(BDRVSheepdogState *s, Error **errp) return fd; } +/* + * Parse numeric snapshot ID in @str + * If @str can't be parsed as number, return false. + * Else, if the number is zero or too large, set *@snapid to zero and + * return true. + * Else, set *@snapid to the number and return true. + */ +static bool sd_parse_snapid(const char *str, uint32_t *snapid) +{ + unsigned long ul; + int ret; + + ret = qemu_strtoul(str, NULL, 10, &ul); + if (ret == -ERANGE) { + ul = ret = 0; + } + if (ret) { + return false; + } + if (ul > UINT32_MAX) { + ul = 0; + } + + *snapid = ul; + return true; +} + +static bool sd_parse_snapid_or_tag(const char *str, + uint32_t *snapid, char tag[]) +{ + if (!sd_parse_snapid(str, snapid)) { + *snapid = 0; + if (g_strlcpy(tag, str, SD_MAX_VDI_TAG_LEN) >= SD_MAX_VDI_TAG_LEN) { + return false; + } + } else if (!*snapid) { + return false; + } else { + tag[0] = 0; + } + return true; +} + static int sd_parse_uri(BDRVSheepdogState *s, const char *filename, char *vdi, uint32_t *snapid, char *tag) { @@ -965,9 +1008,9 @@ static int sd_parse_uri(BDRVSheepdogState *s, const char *filename, /* snapshot tag */ if (uri->fragment) { - *snapid = strtoul(uri->fragment, NULL, 10); - if (*snapid == 0) { - pstrcpy(tag, SD_MAX_VDI_TAG_LEN, uri->fragment); + if (!sd_parse_snapid_or_tag(uri->fragment, snapid, tag)) { + ret = -EINVAL; + goto out; } } else { *snapid = CURRENT_VDI_ID; /* search current vdi */ @@ -1686,6 +1729,7 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt) } copy = strtol(n1, NULL, 10); + /* FIXME fix error checking by switching to qemu_strtol() */ if (copy > SD_MAX_COPIES || copy < 1) { return -EINVAL; } @@ -1700,6 +1744,7 @@ static int parse_redundancy(BDRVSheepdogState *s, const char *opt) } parity = strtol(n2, NULL, 10); + /* FIXME fix error checking by switching to qemu_strtol() */ if (parity >= SD_EC_MAX_STRIP || parity < 1) { return -EINVAL; } @@ -2366,19 +2411,16 @@ static int sd_snapshot_goto(BlockDriverState *bs, const char *snapshot_id) BDRVSheepdogState *old_s; char tag[SD_MAX_VDI_TAG_LEN]; uint32_t snapid = 0; - int ret = 0; + int ret; + + if (!sd_parse_snapid_or_tag(snapshot_id, &snapid, tag)) { + return -EINVAL; + } old_s = g_new(BDRVSheepdogState, 1); memcpy(old_s, s, sizeof(BDRVSheepdogState)); - snapid = strtoul(snapshot_id, NULL, 10); - if (snapid) { - tag[0] = 0; - } else { - pstrcpy(tag, sizeof(tag), snapshot_id); - } - ret = reload_inode(s, snapid, tag); if (ret) { goto out; @@ -2483,6 +2525,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, memset(buf, 0, sizeof(buf)); memset(snap_tag, 0, sizeof(snap_tag)); pstrcpy(buf, SD_MAX_VDI_LEN, s->name); + /* TODO Use sd_parse_snapid() once this mess is cleaned up */ ret = qemu_strtoul(snapshot_id, NULL, 10, &snap_id); if (ret || snap_id > UINT32_MAX) { /* @@ -2499,6 +2542,7 @@ static int sd_snapshot_delete(BlockDriverState *bs, hdr.snapid = (uint32_t) snap_id; } else { /* FIXME I suspect we should use @name here */ + /* FIXME don't truncate silently */ pstrcpy(snap_tag, sizeof(snap_tag), snapshot_id); pstrcpy(buf + SD_MAX_VDI_LEN, SD_MAX_VDI_TAG_LEN, snap_tag); }
sd_parse_uri() and sd_snapshot_goto() screw up error checking after strtoul(), and truncate long tag names silently. Fix by replacing those parts by new sd_parse_snapid_or_tag(), which checks more carefully. sd_snapshot_delete() also parses snapshot IDs, but is currently too broken for me to touch. Mark TODO. Two calls of strtol() without error checking remain in parse_redundancy(). Mark them FIXME. More silent truncation of configuration strings remains elsewhere. Not marked. Signed-off-by: Markus Armbruster <armbru@redhat.com> --- block/sheepdog.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++---------- 1 file changed, 55 insertions(+), 11 deletions(-)