[v3] mesa/util: add missing va_end() after va_copy()

Submitted by andrey simiklit on Sept. 4, 2018, 2:35 p.m.

Details

Message ID 1536071703-30372-1-git-send-email-asimiklit.work@gmail.com
State New
Headers show
Series "mesa/util: add missing va_end() after va_copy()" ( rev: 3 ) in Mesa

Not browsing as part of any series.

Commit Message

andrey simiklit Sept. 4, 2018, 2:35 p.m.
From: Andrii Simiklit <andrii.simiklit@globallogic.com>

MSDN:
"va_end must be called on each argument list that's initialized
 with va_start or va_copy before the function returns."

v2: Linux man about vXXXprintf functions:
    "These functions do not call the va_end macro. Because they
     invoke the va_arg  macro, the value of ap is undefined after the call"
    So we should have instance copy of va_list for each 'vXXXprintf' call.

v3: Fixed case when malloc returns 0 in util_vasprintf

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107810
Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>
---
 src/util/u_string.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/util/u_string.h b/src/util/u_string.h
index ce45430..e408146 100644
--- a/src/util/u_string.h
+++ b/src/util/u_string.h
@@ -81,6 +81,7 @@  util_vsnprintf(char *str, size_t size, const char *format, va_list ap)
    if (ret < 0) {
       ret = _vscprintf(format, ap_copy);
    }
+   va_end(ap_copy);
    return ret;
 }
 
@@ -119,14 +120,14 @@  util_vasprintf(char **ret, const char *format, va_list ap)
 
    /* Compute length of output string first */
    va_copy(ap_copy, ap);
-   int r = util_vsnprintf(NULL, 0, format, ap);
+   int r = util_vsnprintf(NULL, 0, format, ap_copy);
    va_end(ap_copy);
 
    if (r < 0)
       return -1;
 
    *ret = (char *) malloc(r + 1);
-   if (!ret)
+   if (!*ret)
       return -1;
 
    /* Print to buffer */

Comments

On Tuesday, 2018-09-04 17:35:03 +0300, asimiklit.work@gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> 
> MSDN:
> "va_end must be called on each argument list that's initialized
>  with va_start or va_copy before the function returns."
> 
> v2: Linux man about vXXXprintf functions:
>     "These functions do not call the va_end macro. Because they
>      invoke the va_arg  macro, the value of ap is undefined after the call"
>     So we should have instance copy of va_list for each 'vXXXprintf' call.
> 
> v3: Fixed case when malloc returns 0 in util_vasprintf
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107810
> Signed-off-by: Andrii Simiklit <andrii.simiklit@globallogic.com>

Each of these hunks is
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>

But please split them into separate patches, they are unrelated to
eachother :)

If you can, try to find for each the commit that broke it (or introduced
already-broken code) and add `Fixes: $HASH "$COMMIT_MSG"`; if you can't,
just add:
Cc: mesa-stable@lists.freedesktop.org

> ---
>  src/util/u_string.h | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/u_string.h b/src/util/u_string.h
> index ce45430..e408146 100644
> --- a/src/util/u_string.h
> +++ b/src/util/u_string.h
> @@ -81,6 +81,7 @@ util_vsnprintf(char *str, size_t size, const char *format, va_list ap)
>     if (ret < 0) {
>        ret = _vscprintf(format, ap_copy);
>     }
> +   va_end(ap_copy);
>     return ret;
>  }
>  
> @@ -119,14 +120,14 @@ util_vasprintf(char **ret, const char *format, va_list ap)
>  
>     /* Compute length of output string first */
>     va_copy(ap_copy, ap);
> -   int r = util_vsnprintf(NULL, 0, format, ap);
> +   int r = util_vsnprintf(NULL, 0, format, ap_copy);
>     va_end(ap_copy);
>  
>     if (r < 0)
>        return -1;
>  
>     *ret = (char *) malloc(r + 1);
> -   if (!ret)
> +   if (!*ret)
>        return -1;
>  
>     /* Print to buffer */
> -- 
> 2.7.4
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
From: Andrii Simiklit <andrii.simiklit@globallogic.com>

This patch improve usage of [v][s][n]printf functions.
Fixes cross platform implementation of vsnprintf

v2: Linux man about vXXXprintf functions:
    "These functions do not call the va_end macro. Because they
     invoke the va_arg  macro, the value of ap is undefined after the call"
     So we should have instance copy of va_list for each 'vXXXprintf' call.

v3: Fixed case when malloc returns 0 in util_vasprintf

v4: The single patch was separated to the patch series

Andrii Simiklit (8):
  apple/glx/log: added missing va_end() after va_copy()
  mesa/util: don't use the same 'va_list' instance twice
  mesa/util: don't ignore NULL returned from 'malloc'
  mesa/util: add missing va_end() after va_copy()
  mesa/main: don't use win _vsnprintf. there is 'util_vsnprintf'
    function
  mesa/util: use cross platform implementation 'util_vsnprintf'
  radv: use cross platform implementation 'util_vsnprintf'
  egl/main: use cross platform implementation 'util_vsnprintf'

 src/amd/vulkan/radv_util.c    |  5 +++--
 src/egl/main/egllog.c         |  3 ++-
 src/glx/apple/apple_glx_log.c |  1 +
 src/mesa/main/imports.c       | 11 +++++------
 src/util/ralloc.c             | 25 ++++++-------------------
 src/util/u_string.h           | 10 ++++++++--
 6 files changed, 25 insertions(+), 30 deletions(-)
On Thursday, 2018-09-06 17:43:50 +0300, asimiklit.work@gmail.com wrote:
> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> 
> This patch improve usage of [v][s][n]printf functions.
> Fixes cross platform implementation of vsnprintf
> 
> v2: Linux man about vXXXprintf functions:
>     "These functions do not call the va_end macro. Because they
>      invoke the va_arg  macro, the value of ap is undefined after the call"
>      So we should have instance copy of va_list for each 'vXXXprintf' call.
> 
> v3: Fixed case when malloc returns 0 in util_vasprintf
> 
> v4: The single patch was separated to the patch series
> 
> Andrii Simiklit (8):
>   apple/glx/log: added missing va_end() after va_copy()
>   mesa/util: don't use the same 'va_list' instance twice
>   mesa/util: don't ignore NULL returned from 'malloc'
>   mesa/util: add missing va_end() after va_copy()

First 4 are
Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
and pushed :)

Thanks for taking the time to find the right Fixes: btw, that really
helps when backporting to stable branches :)

>   mesa/main: don't use win _vsnprintf. there is 'util_vsnprintf'
>     function
>   mesa/util: use cross platform implementation 'util_vsnprintf'

These will require more time than I can give right now; maybe later if
nobody beats me to it :)

>   radv: use cross platform implementation 'util_vsnprintf'
>   egl/main: use cross platform implementation 'util_vsnprintf'

These two are:
Acked-by: Eric Engestrom <eric.engestrom@intel.com>

And I'll do all the double-checking later if I can and turn them into
r-bs :P

> 
>  src/amd/vulkan/radv_util.c    |  5 +++--
>  src/egl/main/egllog.c         |  3 ++-
>  src/glx/apple/apple_glx_log.c |  1 +
>  src/mesa/main/imports.c       | 11 +++++------
>  src/util/ralloc.c             | 25 ++++++-------------------
>  src/util/u_string.h           | 10 ++++++++--
>  6 files changed, 25 insertions(+), 30 deletions(-)
> 
> -- 
> 2.7.4
>
On Thu, 2018-09-06 at 17:58 +0100, Eric Engestrom wrote:
> On Thursday, 2018-09-06 17:43:50 +0300, asimiklit.work@gmail.com wrote:
> > From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> > 
> > This patch improve usage of [v][s][n]printf functions.
> > Fixes cross platform implementation of vsnprintf
> > 
> > v2: Linux man about vXXXprintf functions:
> >     "These functions do not call the va_end macro. Because they
> >      invoke the va_arg  macro, the value of ap is undefined after the call"
> >      So we should have instance copy of va_list for each 'vXXXprintf' call.
> > 
> > v3: Fixed case when malloc returns 0 in util_vasprintf
> > 
> > v4: The single patch was separated to the patch series
> > 
> > Andrii Simiklit (8):
> >   apple/glx/log: added missing va_end() after va_copy()
> >   mesa/util: don't use the same 'va_list' instance twice
> >   mesa/util: don't ignore NULL returned from 'malloc'
> >   mesa/util: add missing va_end() after va_copy()
> 
> First 4 are
> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
> and pushed :)
> 
> Thanks for taking the time to find the right Fixes: btw, that really
> helps when backporting to stable branches :)

Indeed ☺

Thanks Andrii for the effort, and Eric for the advice to split and tag
with the proper Fixes.
On Thu, Sep 6, 2018 at 7:58 PM Eric Engestrom <eric.engestrom@intel.com>
wrote:

> On Thursday, 2018-09-06 17:43:50 +0300, asimiklit.work@gmail.com wrote:
> > From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> >
> > This patch improve usage of [v][s][n]printf functions.
> > Fixes cross platform implementation of vsnprintf
> >
> > v2: Linux man about vXXXprintf functions:
> >     "These functions do not call the va_end macro. Because they
> >      invoke the va_arg  macro, the value of ap is undefined after the
> call"
> >      So we should have instance copy of va_list for each 'vXXXprintf'
> call.
> >
> > v3: Fixed case when malloc returns 0 in util_vasprintf
> >
> > v4: The single patch was separated to the patch series
> >
> > Andrii Simiklit (8):
> >   apple/glx/log: added missing va_end() after va_copy()
> >   mesa/util: don't use the same 'va_list' instance twice
> >   mesa/util: don't ignore NULL returned from 'malloc'
> >   mesa/util: add missing va_end() after va_copy()
>
> First 4 are
> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
> and pushed :)
>
> Thanks for taking the time to find the right Fixes: btw, that really
> helps when backporting to stable branches :)
>
> >   mesa/main: don't use win _vsnprintf. there is 'util_vsnprintf'
> >     function
> >   mesa/util: use cross platform implementation 'util_vsnprintf'
>
> These will require more time than I can give right now; maybe later if
> nobody beats me to it :)
>
> >   radv: use cross platform implementation 'util_vsnprintf'
> >   egl/main: use cross platform implementation 'util_vsnprintf'
>
> These two are:
> Acked-by: Eric Engestrom <eric.engestrom@intel.com>
>
> And I'll do all the double-checking later if I can and turn them into
> r-bs :P
>

Thanks a lot for your reviewing)


> >
> >  src/amd/vulkan/radv_util.c    |  5 +++--
> >  src/egl/main/egllog.c         |  3 ++-
> >  src/glx/apple/apple_glx_log.c |  1 +
> >  src/mesa/main/imports.c       | 11 +++++------
> >  src/util/ralloc.c             | 25 ++++++-------------------
> >  src/util/u_string.h           | 10 ++++++++--
> >  6 files changed, 25 insertions(+), 30 deletions(-)
> >
> > --
> > 2.7.4
> >
>
On 6 September 2018 at 17:58, Eric Engestrom <eric.engestrom@intel.com> wrote:
> On Thursday, 2018-09-06 17:43:50 +0300, asimiklit.work@gmail.com wrote:
>> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
>>
>> This patch improve usage of [v][s][n]printf functions.
>> Fixes cross platform implementation of vsnprintf
>>
>> v2: Linux man about vXXXprintf functions:
>>     "These functions do not call the va_end macro. Because they
>>      invoke the va_arg  macro, the value of ap is undefined after the call"
>>      So we should have instance copy of va_list for each 'vXXXprintf' call.
>>
>> v3: Fixed case when malloc returns 0 in util_vasprintf
>>
>> v4: The single patch was separated to the patch series
>>
>> Andrii Simiklit (8):
>>   apple/glx/log: added missing va_end() after va_copy()
>>   mesa/util: don't use the same 'va_list' instance twice
>>   mesa/util: don't ignore NULL returned from 'malloc'
>>   mesa/util: add missing va_end() after va_copy()
>
> First 4 are
> Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
> and pushed :)
>
> Thanks for taking the time to find the right Fixes: btw, that really
> helps when backporting to stable branches :)
>
Small nitpick: "mesa/util" should be just "util".
Regardless, patches are spot on. Kudos for all the Fixes tags.

>>   mesa/main: don't use win _vsnprintf. there is 'util_vsnprintf'
>>     function
As-is, or reworked to drop the _mesa_ function (as Ian suggested) patch is
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

>>   mesa/util: use cross platform implementation 'util_vsnprintf'
>
"mesa/util" -> "util", with that
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>


>>   radv: use cross platform implementation 'util_vsnprintf'
>>   egl/main: use cross platform implementation 'util_vsnprintf'
>
> These two are:
> Acked-by: Eric Engestrom <eric.engestrom@intel.com>
>
Reviewed-by: Emil Velikov <emil.velikov@collabora.com>

Admittedly, I doubt we'll get radv on Windows soon. At least not
before we bump the requirement to MSVC2015 which has C99 compliant
function.

HTH
Emil
On Fri, Sep 7, 2018 at 12:09 PM Emil Velikov <emil.l.velikov@gmail.com>
wrote:

> On 6 September 2018 at 17:58, Eric Engestrom <eric.engestrom@intel.com>
> wrote:
> > On Thursday, 2018-09-06 17:43:50 +0300, asimiklit.work@gmail.com wrote:
> >> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
> >>
> >> This patch improve usage of [v][s][n]printf functions.
> >> Fixes cross platform implementation of vsnprintf
> >>
> >> v2: Linux man about vXXXprintf functions:
> >>     "These functions do not call the va_end macro. Because they
> >>      invoke the va_arg  macro, the value of ap is undefined after the
> call"
> >>      So we should have instance copy of va_list for each 'vXXXprintf'
> call.
> >>
> >> v3: Fixed case when malloc returns 0 in util_vasprintf
> >>
> >> v4: The single patch was separated to the patch series
> >>
> >> Andrii Simiklit (8):
> >>   apple/glx/log: added missing va_end() after va_copy()
> >>   mesa/util: don't use the same 'va_list' instance twice
> >>   mesa/util: don't ignore NULL returned from 'malloc'
> >>   mesa/util: add missing va_end() after va_copy()
> >
> > First 4 are
> > Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
> > and pushed :)
> >
> > Thanks for taking the time to find the right Fixes: btw, that really
> > helps when backporting to stable branches :)
> >
> Small nitpick: "mesa/util" should be just "util".
> Regardless, patches are spot on. Kudos for all the Fixes tags.
>
> >>   mesa/main: don't use win _vsnprintf. there is 'util_vsnprintf'
> >>     function
> As-is, or reworked to drop the _mesa_ function (as Ian suggested) patch is
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>

Changes are in progress. Thanks for reviewing.


> >>   mesa/util: use cross platform implementation 'util_vsnprintf'
> >
> "mesa/util" -> "util", with that
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>

I will change it.


>
>
> >>   radv: use cross platform implementation 'util_vsnprintf'
> >>   egl/main: use cross platform implementation 'util_vsnprintf'
> >
> > These two are:
> > Acked-by: Eric Engestrom <eric.engestrom@intel.com>
> >
> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>
> Admittedly, I doubt we'll get radv on Windows soon. At least not
> before we bump the requirement to MSVC2015 which has C99 compliant
> function.
>

Agree)
It would be great if it was similar to other places of the mesa code.
Thanks for reviewing.


> HTH
> Emil
>
Hi all,

Could somebody helps me push the remaining patches which were already
reviewed:

1. [PATCH v5 5/8] mesa/main: don't use win _vsnprintf. there is
'util_vsnprintf' function
    https://patchwork.freedesktop.org/patch/247749/
2. [PATCH v5 6/8] util: use cross platform implementation 'util_vsnprintf'
    https://patchwork.freedesktop.org/patch/247748/
3. [PATCH v4 7/8] radv: use cross platform implementation 'util_vsnprintf'
    https://patchwork.freedesktop.org/patch/247611/
4. [PATCH v4 8/8] egl/main: use cross platform implementation
'util_vsnprintf'
    https://patchwork.freedesktop.org/patch/247612/

Regards,
Andrii.
On Fri, Sep 7, 2018 at 12:42 PM andrey simiklit <asimiklit.work@gmail.com>
wrote:

> On Fri, Sep 7, 2018 at 12:09 PM Emil Velikov <emil.l.velikov@gmail.com>
> wrote:
>
>> On 6 September 2018 at 17:58, Eric Engestrom <eric.engestrom@intel.com>
>> wrote:
>> > On Thursday, 2018-09-06 17:43:50 +0300, asimiklit.work@gmail.com wrote:
>> >> From: Andrii Simiklit <andrii.simiklit@globallogic.com>
>> >>
>> >> This patch improve usage of [v][s][n]printf functions.
>> >> Fixes cross platform implementation of vsnprintf
>> >>
>> >> v2: Linux man about vXXXprintf functions:
>> >>     "These functions do not call the va_end macro. Because they
>> >>      invoke the va_arg  macro, the value of ap is undefined after the
>> call"
>> >>      So we should have instance copy of va_list for each 'vXXXprintf'
>> call.
>> >>
>> >> v3: Fixed case when malloc returns 0 in util_vasprintf
>> >>
>> >> v4: The single patch was separated to the patch series
>> >>
>> >> Andrii Simiklit (8):
>> >>   apple/glx/log: added missing va_end() after va_copy()
>> >>   mesa/util: don't use the same 'va_list' instance twice
>> >>   mesa/util: don't ignore NULL returned from 'malloc'
>> >>   mesa/util: add missing va_end() after va_copy()
>> >
>> > First 4 are
>> > Reviewed-by: Eric Engestrom <eric.engestrom@intel.com>
>> > and pushed :)
>> >
>> > Thanks for taking the time to find the right Fixes: btw, that really
>> > helps when backporting to stable branches :)
>> >
>> Small nitpick: "mesa/util" should be just "util".
>> Regardless, patches are spot on. Kudos for all the Fixes tags.
>>
>> >>   mesa/main: don't use win _vsnprintf. there is 'util_vsnprintf'
>> >>     function
>> As-is, or reworked to drop the _mesa_ function (as Ian suggested) patch is
>> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>>
>
> Changes are in progress. Thanks for reviewing.
>
>
>> >>   mesa/util: use cross platform implementation 'util_vsnprintf'
>> >
>> "mesa/util" -> "util", with that
>> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>>
>
> I will change it.
>
>
>>
>>
>> >>   radv: use cross platform implementation 'util_vsnprintf'
>> >>   egl/main: use cross platform implementation 'util_vsnprintf'
>> >
>> > These two are:
>> > Acked-by: Eric Engestrom <eric.engestrom@intel.com>
>> >
>> Reviewed-by: Emil Velikov <emil.velikov@collabora.com>
>>
>> Admittedly, I doubt we'll get radv on Windows soon. At least not
>> before we bump the requirement to MSVC2015 which has C99 compliant
>> function.
>>
>
> Agree)
> It would be great if it was similar to other places of the mesa code.
> Thanks for reviewing.
>
>
>> HTH
>> Emil
>>
>