Thanks for your reply. > Really? How exactly can this happen? Please explain step by step. There exist a qemu core related to this. You have mention that "The conversion truncates when strlen(str) - 1 exceeds INT_MAX". Later in function qstring_from_substr, this truncated "end" will be assigned to "qstring->length" again, which is size_t. This is the key point why qemu coredumped. Because when "end" is truncated, it can be negative number. If we assign a negative number to a size_t variable, this size_t variable can become very large. At last, we call g_malloc to try to alloc a large number of member which cannot success. So qemu coredump. In my example, use gdb to debug function qstring_from_substr, I can get the following message. (gdb) p qstring->length $4 = 18446744072383980732 (too large to allocate) (gdb) p (int) (qstring->length) $5 = -1325570884 (gdb) p/x (int) qstring->length $6 = 0xb0fd64bc (gdb) p/x qstring->length $7 = 0xffffffffb0fd64bc (gdb) p end $8 = <optimized out>
> -----Original Message----- > From: Markus Armbruster [mailto:arm...@redhat.com] > Sent: Monday, July 23, 2018 8:48 PM > To: liujunjie (A) <liujunji...@huawei.com> > Cc: wangxin (U) <wangxinxin.w...@huawei.com>; Gonglei (Arei) > <arei.gong...@huawei.com>; Huangweidong (C) > <weidong.hu...@huawei.com>; qemu-devel@nongnu.org > Subject: Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow > > liujunjie <liujunji...@huawei.com> writes: > > > From: l00425170 <liujunji...@huawei.com> > > > > The incoming parameters "start" and "end" is int type in > > qstring_from_substr(), but this function can be called by > > qstring_from_str, which is size_t type in strlen(str). > > Yes, there's a conversion from size_t to int in > > return qstring_from_substr(str, 0, strlen(str) - 1); > > The conversion truncates when strlen(str) - 1 exceeds INT_MAX. > > strlen(str) - 1 wraps around to SIZE_MAX when @str is empty. > > > It may result in coredump when called g_malloc later. > > One scene to triger is to call hmp "into tlb", which may have too long > > length of string. > > Really? How exactly can this happen? Please explain step by step. > > Aside: @end is only used as @end + 1, and all callers pass some X - 1. > Both the addition and the subtraction can overflow. Changing the function to > take the index behind the last character instead of the index of the last > character would almost certainly simplify things. Not this patch's problem. > > > > > Signed-off-by: l00425170 <liujunji...@huawei.com> > > --- > > include/qapi/qmp/qstring.h | 2 +- > > qobject/qstring.c | 2 +- > > 2 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/include/qapi/qmp/qstring.h b/include/qapi/qmp/qstring.h > > index b3b3d44..3e83e3a 100644 > > --- a/include/qapi/qmp/qstring.h > > +++ b/include/qapi/qmp/qstring.h > > @@ -24,7 +24,7 @@ struct QString { > > > > QString *qstring_new(void); > > QString *qstring_from_str(const char *str); -QString > > *qstring_from_substr(const char *str, int start, int end); > > +QString *qstring_from_substr(const char *str, size_t start, size_t > > +end); > > size_t qstring_get_length(const QString *qstring); const char > > *qstring_get_str(const QString *qstring); const char > > *qstring_get_try_str(const QString *qstring); diff --git > > a/qobject/qstring.c b/qobject/qstring.c index afca54b..18b8eb8 100644 > > --- a/qobject/qstring.c > > +++ b/qobject/qstring.c > > @@ -37,7 +37,7 @@ size_t qstring_get_length(const QString *qstring) > > * > > * Return string reference > > */ > > -QString *qstring_from_substr(const char *str, int start, int end) > > +QString *qstring_from_substr(const char *str, size_t start, size_t > > +end) > > { > > QString *qstring; > > The patch makes sense, but the commit message makes claims that need to be > substantiated.
