Re: [Qemu-devel] [PATCH] qstring: Fix integer overflow

[Markus Armbruster <armbru@redhat.com>]

liujunjie <liujunjie23@huawei.com> writes:

> From: l00425170 <liujunjie23@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 <liujunjie23@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.