Message ID | 20180311201239.25506-1-nia.alarie@gmail.com |
---|---|
State | New |
Headers | show |
Series | 9p: Convert use of atoi to qemu_strtol to allow error checking | expand |
On Sun, 11 Mar 2018 20:12:39 +0000 Nia Alarie <nia.alarie@gmail.com> wrote: > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> > --- Applied, thanks. > hw/9pfs/9p.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > index 48fa48e720..64f3bb976c 100644 > --- a/hw/9pfs/9p.c > +++ b/hw/9pfs/9p.c > @@ -15,6 +15,7 @@ > #include <glib/gprintf.h> > #include "hw/virtio/virtio.h" > #include "qapi/error.h" > +#include "qemu/cutils.h" > #include "qemu/error-report.h" > #include "qemu/iov.h" > #include "qemu/sockets.h" > @@ -2213,8 +2214,15 @@ static void coroutine_fn v9fs_create(void *opaque) > } > v9fs_path_copy(&fidp->path, &path); > } else if (perm & P9_STAT_MODE_LINK) { > - int32_t ofid = atoi(extension.data); > - V9fsFidState *ofidp = get_fid(pdu, ofid); > + long ofid; > + V9fsFidState *ofidp; > + > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || > + ofid > INT32_MAX || ofid < INT32_MIN) { > + err = -EINVAL; > + goto out; > + } > + ofidp = get_fid(pdu, (int32_t)ofid); > if (ofidp == NULL) { > err = -EINVAL; > goto out;
On 03/11/2018 03:12 PM, Nia Alarie wrote: > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> > --- > hw/9pfs/9p.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > } else if (perm & P9_STAT_MODE_LINK) { > - int32_t ofid = atoi(extension.data); > - V9fsFidState *ofidp = get_fid(pdu, ofid); > + long ofid; > + V9fsFidState *ofidp; > + > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || > + ofid > INT32_MAX || ofid < INT32_MIN) { Dan has a pending patch that will add qemu_strtoi, which might be a nicer fit for this situation: https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00952.html int32_t is not necessarily int, but all platforms that compile qemu have 'int32_t' and 'int' both at 32 bits, so it's simpler to change to 'int ofid' and use Dan's function than it is to parse to long and then do bounds checking. Except that Dan still needs to post an updated version of his patch...
On Mon, 12 Mar 2018 07:12:52 -0500 Eric Blake <eblake@redhat.com> wrote: > On 03/11/2018 03:12 PM, Nia Alarie wrote: > > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> > > --- > > hw/9pfs/9p.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > } else if (perm & P9_STAT_MODE_LINK) { > > - int32_t ofid = atoi(extension.data); > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > > + long ofid; > > + V9fsFidState *ofidp; > > + > > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || > > + ofid > INT32_MAX || ofid < INT32_MIN) { > > Dan has a pending patch that will add qemu_strtoi, which might be a > nicer fit for this situation: > > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00952.html > > int32_t is not necessarily int, but all platforms that compile qemu have > 'int32_t' and 'int' both at 32 bits, so it's simpler to change to 'int > ofid' and use Dan's function than it is to parse to long and then do > bounds checking. Except that Dan still needs to post an updated version > of his patch... > I wasn't aware of Dan's patch but I agree it would result in a nicer change for 9p. This being said, tomorrow is soft freeze... is there a chance Dan's patch reaches master anytime soon ?
On Mon, Mar 12, 2018 at 02:02:29PM +0100, Greg Kurz wrote: > On Mon, 12 Mar 2018 07:12:52 -0500 > Eric Blake <eblake@redhat.com> wrote: > > > On 03/11/2018 03:12 PM, Nia Alarie wrote: > > > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> > > > --- > > > hw/9pfs/9p.c | 12 ++++++++++-- > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > } else if (perm & P9_STAT_MODE_LINK) { > > > - int32_t ofid = atoi(extension.data); > > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > > > + long ofid; > > > + V9fsFidState *ofidp; > > > + > > > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || > > > + ofid > INT32_MAX || ofid < INT32_MIN) { > > > > Dan has a pending patch that will add qemu_strtoi, which might be a > > nicer fit for this situation: > > > > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00952.html > > > > int32_t is not necessarily int, but all platforms that compile qemu have > > 'int32_t' and 'int' both at 32 bits, so it's simpler to change to 'int > > ofid' and use Dan's function than it is to parse to long and then do > > bounds checking. Except that Dan still needs to post an updated version > > of his patch... > > > > I wasn't aware of Dan's patch but I agree it would result in a nicer > change for 9p. This being said, tomorrow is soft freeze... is there > a chance Dan's patch reaches master anytime soon ? I just sent an update of my series. If it gets acked by Eric, I'd be able to send a pull request today. Regards, Daniel
On Mon, 12 Mar 2018 10:01:46 +0100 Greg Kurz <groug@kaod.org> wrote: > On Sun, 11 Mar 2018 20:12:39 +0000 > Nia Alarie <nia.alarie@gmail.com> wrote: > > > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> > > --- > > Applied, thanks. > Following Eric's suggestion in another mail, let's give a chance for the new qemu_strto*() helpers to reach master. Also, FIDs are unsigned 32-bit integers, so we should use a qemu_strtou*() variant. > > hw/9pfs/9p.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c > > index 48fa48e720..64f3bb976c 100644 > > --- a/hw/9pfs/9p.c > > +++ b/hw/9pfs/9p.c > > @@ -15,6 +15,7 @@ > > #include <glib/gprintf.h> > > #include "hw/virtio/virtio.h" > > #include "qapi/error.h" > > +#include "qemu/cutils.h" > > #include "qemu/error-report.h" > > #include "qemu/iov.h" > > #include "qemu/sockets.h" > > @@ -2213,8 +2214,15 @@ static void coroutine_fn v9fs_create(void *opaque) > > } > > v9fs_path_copy(&fidp->path, &path); > > } else if (perm & P9_STAT_MODE_LINK) { > > - int32_t ofid = atoi(extension.data); > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > > + long ofid; > > + V9fsFidState *ofidp; > > + > > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || > > + ofid > INT32_MAX || ofid < INT32_MIN) { > > + err = -EINVAL; > > + goto out; > > + } > > + ofidp = get_fid(pdu, (int32_t)ofid); > > if (ofidp == NULL) { > > err = -EINVAL; > > goto out; > >
On Mon, 12 Mar 2018 13:08:29 +0000 Daniel P. Berrangé <berrange@redhat.com> wrote: > On Mon, Mar 12, 2018 at 02:02:29PM +0100, Greg Kurz wrote: > > On Mon, 12 Mar 2018 07:12:52 -0500 > > Eric Blake <eblake@redhat.com> wrote: > > > > > On 03/11/2018 03:12 PM, Nia Alarie wrote: > > > > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> > > > > --- > > > > hw/9pfs/9p.c | 12 ++++++++++-- > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > > > > } else if (perm & P9_STAT_MODE_LINK) { > > > > - int32_t ofid = atoi(extension.data); > > > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > > > > + long ofid; > > > > + V9fsFidState *ofidp; > > > > + > > > > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || > > > > + ofid > INT32_MAX || ofid < INT32_MIN) { > > > > > > Dan has a pending patch that will add qemu_strtoi, which might be a > > > nicer fit for this situation: > > > > > > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00952.html > > > > > > int32_t is not necessarily int, but all platforms that compile qemu have > > > 'int32_t' and 'int' both at 32 bits, so it's simpler to change to 'int > > > ofid' and use Dan's function than it is to parse to long and then do > > > bounds checking. Except that Dan still needs to post an updated version > > > of his patch... > > > > > > > I wasn't aware of Dan's patch but I agree it would result in a nicer > > change for 9p. This being said, tomorrow is soft freeze... is there > > a chance Dan's patch reaches master anytime soon ? > > I just sent an update of my series. If it gets acked by Eric, I'd be able > to send a pull request today. > > Regards, > Daniel Great ! Nia, Could you please respin your patch on top of Daniel's series, using qemu_strtoui() ? Thanks, -- Greg
On Mon, Mar 12, 2018 at 1:21 PM, Greg Kurz <groug@kaod.org> wrote: > On Mon, 12 Mar 2018 13:08:29 +0000 > Daniel P. Berrangé <berrange@redhat.com> wrote: > >> On Mon, Mar 12, 2018 at 02:02:29PM +0100, Greg Kurz wrote: >> > On Mon, 12 Mar 2018 07:12:52 -0500 >> > Eric Blake <eblake@redhat.com> wrote: >> > >> > > On 03/11/2018 03:12 PM, Nia Alarie wrote: >> > > > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> >> > > > --- >> > > > hw/9pfs/9p.c | 12 ++++++++++-- >> > > > 1 file changed, 10 insertions(+), 2 deletions(-) >> > > > >> > > >> > > > } else if (perm & P9_STAT_MODE_LINK) { >> > > > - int32_t ofid = atoi(extension.data); >> > > > - V9fsFidState *ofidp = get_fid(pdu, ofid); >> > > > + long ofid; >> > > > + V9fsFidState *ofidp; >> > > > + >> > > > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || >> > > > + ofid > INT32_MAX || ofid < INT32_MIN) { >> > > >> > > Dan has a pending patch that will add qemu_strtoi, which might be a >> > > nicer fit for this situation: >> > > >> > > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00952.html >> > > >> > > int32_t is not necessarily int, but all platforms that compile qemu have >> > > 'int32_t' and 'int' both at 32 bits, so it's simpler to change to 'int >> > > ofid' and use Dan's function than it is to parse to long and then do >> > > bounds checking. Except that Dan still needs to post an updated version >> > > of his patch... >> > > >> > >> > I wasn't aware of Dan's patch but I agree it would result in a nicer >> > change for 9p. This being said, tomorrow is soft freeze... is there >> > a chance Dan's patch reaches master anytime soon ? >> >> I just sent an update of my series. If it gets acked by Eric, I'd be able >> to send a pull request today. >> >> Regards, >> Daniel > > Great ! > > Nia, > > Could you please respin your patch on top of Daniel's series, using > qemu_strtoui() ? > > Thanks, > > -- > Greg I'm a bit unclear on how this works so I thought I'd ask here before I send any more patches - this is CCed to my mentors as well. It's "tomorrow" now, but I don't understand how soft freezes work or the process beyond "I send this patch to qemu-devel and people say if I should re-send it with changes, or that it's been accepted" :) There are several more conversions from ato* to qemu_strto* I'd like to submit from now on. Should I send them as using qemu_strtol now, to get them "out there" before my own personal deadlines, and send further patches to convert them to qemu_strtoi later, or should I submit patches that use qemu_strtoi? I'm not sure of the current status of Daniel's patches. Thank you.
On Tue, Mar 13, 2018 at 3:25 PM, nee <nia.alarie@gmail.com> wrote: > On Mon, Mar 12, 2018 at 1:21 PM, Greg Kurz <groug@kaod.org> wrote: >> On Mon, 12 Mar 2018 13:08:29 +0000 >> Daniel P. Berrangé <berrange@redhat.com> wrote: >> >>> On Mon, Mar 12, 2018 at 02:02:29PM +0100, Greg Kurz wrote: >>> > On Mon, 12 Mar 2018 07:12:52 -0500 >>> > Eric Blake <eblake@redhat.com> wrote: >>> > >>> > > On 03/11/2018 03:12 PM, Nia Alarie wrote: >>> > > > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> >>> > > > --- >>> > > > hw/9pfs/9p.c | 12 ++++++++++-- >>> > > > 1 file changed, 10 insertions(+), 2 deletions(-) >>> > > > >>> > > >>> > > > } else if (perm & P9_STAT_MODE_LINK) { >>> > > > - int32_t ofid = atoi(extension.data); >>> > > > - V9fsFidState *ofidp = get_fid(pdu, ofid); >>> > > > + long ofid; >>> > > > + V9fsFidState *ofidp; >>> > > > + >>> > > > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || >>> > > > + ofid > INT32_MAX || ofid < INT32_MIN) { >>> > > >>> > > Dan has a pending patch that will add qemu_strtoi, which might be a >>> > > nicer fit for this situation: >>> > > >>> > > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00952.html >>> > > >>> > > int32_t is not necessarily int, but all platforms that compile qemu have >>> > > 'int32_t' and 'int' both at 32 bits, so it's simpler to change to 'int >>> > > ofid' and use Dan's function than it is to parse to long and then do >>> > > bounds checking. Except that Dan still needs to post an updated version >>> > > of his patch... >>> > > >>> > >>> > I wasn't aware of Dan's patch but I agree it would result in a nicer >>> > change for 9p. This being said, tomorrow is soft freeze... is there >>> > a chance Dan's patch reaches master anytime soon ? >>> >>> I just sent an update of my series. If it gets acked by Eric, I'd be able >>> to send a pull request today. >>> >>> Regards, >>> Daniel >> >> Great ! >> >> Nia, >> >> Could you please respin your patch on top of Daniel's series, using >> qemu_strtoui() ? >> >> Thanks, >> >> -- >> Greg > > I'm a bit unclear on how this works so I thought I'd ask here before I > send any more patches - this is CCed to my mentors as well. It's > "tomorrow" now, but I don't understand how soft freezes work or the > process beyond "I send this patch to qemu-devel and people say if I > should re-send it with changes, or that it's been accepted" :) Don't worry about the soft freeze for now. The soft freeze is the deadline for new features. People are trying to get their new features into the upcoming QEMU 2.12 release. Either Greg will still merge your patch after soft freeze since it can be considered a bug fix (fixes are allowed during freeze). Or Greg might keep your patch queued until the QEMU 2.12 release has been made (around April 17th 2018). The QEMU release schedule is here (but don't worry about it): https://wiki.qemu.org/Planning/2.12 I suggest taking the following action. Wait until Daniel Berrange's pull request is merged (it will probably happen today): https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg03662.html Then rebase your patch onto the latest qemu.git/master so you can take advantage of qemu_strtoi(). You can use "git rebase -i origin/master" to do this. Recompile/test your patch and then send it with "[PATCH v2] 9p: Convert use of atoi to qemu_strtoi to allow error checking" as the email subject. > There are several more conversions from ato* to qemu_strto* I'd like > to submit from now on. Should I send them as using qemu_strtol now, to > get them "out there" before my own personal deadlines, and send > further patches to convert them to qemu_strtoi later, or should I > submit patches that use qemu_strtoi? I'm not sure of the current > status of Daniel's patches. Daniel's patches will be in qemu.git/master (probably) later today. I suggest waiting for them and using qemu_strtoi() where using an int type is appropriate. If you have questions along the way, feel free to ask on the QEMU IRC channel: #qemu on irc.oftc.net. Stefan
On Tue, 13 Mar 2018 15:52:41 +0000 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Tue, Mar 13, 2018 at 3:25 PM, nee <nia.alarie@gmail.com> wrote: > > On Mon, Mar 12, 2018 at 1:21 PM, Greg Kurz <groug@kaod.org> wrote: > >> On Mon, 12 Mar 2018 13:08:29 +0000 > >> Daniel P. Berrangé <berrange@redhat.com> wrote: > >> > >>> On Mon, Mar 12, 2018 at 02:02:29PM +0100, Greg Kurz wrote: > >>> > On Mon, 12 Mar 2018 07:12:52 -0500 > >>> > Eric Blake <eblake@redhat.com> wrote: > >>> > > >>> > > On 03/11/2018 03:12 PM, Nia Alarie wrote: > >>> > > > Signed-off-by: Nia Alarie <nia.alarie@gmail.com> > >>> > > > --- > >>> > > > hw/9pfs/9p.c | 12 ++++++++++-- > >>> > > > 1 file changed, 10 insertions(+), 2 deletions(-) > >>> > > > > >>> > > > >>> > > > } else if (perm & P9_STAT_MODE_LINK) { > >>> > > > - int32_t ofid = atoi(extension.data); > >>> > > > - V9fsFidState *ofidp = get_fid(pdu, ofid); > >>> > > > + long ofid; > >>> > > > + V9fsFidState *ofidp; > >>> > > > + > >>> > > > + if (qemu_strtol(extension.data, NULL, 10, &ofid) || > >>> > > > + ofid > INT32_MAX || ofid < INT32_MIN) { > >>> > > > >>> > > Dan has a pending patch that will add qemu_strtoi, which might be a > >>> > > nicer fit for this situation: > >>> > > > >>> > > https://lists.gnu.org/archive/html/qemu-devel/2018-02/msg00952.html > >>> > > > >>> > > int32_t is not necessarily int, but all platforms that compile qemu have > >>> > > 'int32_t' and 'int' both at 32 bits, so it's simpler to change to 'int > >>> > > ofid' and use Dan's function than it is to parse to long and then do > >>> > > bounds checking. Except that Dan still needs to post an updated version > >>> > > of his patch... > >>> > > > >>> > > >>> > I wasn't aware of Dan's patch but I agree it would result in a nicer > >>> > change for 9p. This being said, tomorrow is soft freeze... is there > >>> > a chance Dan's patch reaches master anytime soon ? > >>> > >>> I just sent an update of my series. If it gets acked by Eric, I'd be able > >>> to send a pull request today. > >>> > >>> Regards, > >>> Daniel > >> > >> Great ! > >> > >> Nia, > >> > >> Could you please respin your patch on top of Daniel's series, using > >> qemu_strtoui() ? > >> > >> Thanks, > >> > >> -- > >> Greg > > > > I'm a bit unclear on how this works so I thought I'd ask here before I > > send any more patches - this is CCed to my mentors as well. It's > > "tomorrow" now, but I don't understand how soft freezes work or the > > process beyond "I send this patch to qemu-devel and people say if I > > should re-send it with changes, or that it's been accepted" :) > > Don't worry about the soft freeze for now. The soft freeze is the > deadline for new features. People are trying to get their new > features into the upcoming QEMU 2.12 release. > > Either Greg will still merge your patch after soft freeze since it can > be considered a bug fix (fixes are allowed during freeze). Or Greg > might keep your patch queued until the QEMU 2.12 release has been made > (around April 17th 2018). > Nia (or Nee?) 's patch is a bug fix. I'll happily merge it when it's ready. > The QEMU release schedule is here (but don't worry about it): > https://wiki.qemu.org/Planning/2.12 > > I suggest taking the following action. Wait until Daniel Berrange's > pull request is merged (it will probably happen today): > https://lists.gnu.org/archive/html/qemu-devel/2018-03/msg03662.html > > Then rebase your patch onto the latest qemu.git/master so you can take > advantage of qemu_strtoi(). You can use "git rebase -i origin/master" > to do this. > > Recompile/test your patch and then send it with "[PATCH v2] 9p: > Convert use of atoi to qemu_strtoi to allow error checking" as the > email subject. > Nia had already sent a v2, which got comments, so it'll be v3. > > There are several more conversions from ato* to qemu_strto* I'd like > > to submit from now on. Should I send them as using qemu_strtol now, to > > get them "out there" before my own personal deadlines, and send > > further patches to convert them to qemu_strtoi later, or should I > > submit patches that use qemu_strtoi? I'm not sure of the current > > status of Daniel's patches. > > Daniel's patches will be in qemu.git/master (probably) later today. I > suggest waiting for them and using qemu_strtoi() where using an int > type is appropriate. > > If you have questions along the way, feel free to ask on the QEMU IRC > channel: #qemu on irc.oftc.net. > > Stefan Agreed with all of the above. Thanks Stefan for providing this comprehensive explanation to Nia. Cheers, -- Greg
diff --git a/hw/9pfs/9p.c b/hw/9pfs/9p.c index 48fa48e720..64f3bb976c 100644 --- a/hw/9pfs/9p.c +++ b/hw/9pfs/9p.c @@ -15,6 +15,7 @@ #include <glib/gprintf.h> #include "hw/virtio/virtio.h" #include "qapi/error.h" +#include "qemu/cutils.h" #include "qemu/error-report.h" #include "qemu/iov.h" #include "qemu/sockets.h" @@ -2213,8 +2214,15 @@ static void coroutine_fn v9fs_create(void *opaque) } v9fs_path_copy(&fidp->path, &path); } else if (perm & P9_STAT_MODE_LINK) { - int32_t ofid = atoi(extension.data); - V9fsFidState *ofidp = get_fid(pdu, ofid); + long ofid; + V9fsFidState *ofidp; + + if (qemu_strtol(extension.data, NULL, 10, &ofid) || + ofid > INT32_MAX || ofid < INT32_MIN) { + err = -EINVAL; + goto out; + } + ofidp = get_fid(pdu, (int32_t)ofid); if (ofidp == NULL) { err = -EINVAL; goto out;
Signed-off-by: Nia Alarie <nia.alarie@gmail.com> --- hw/9pfs/9p.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)