[v2,0/3] require newer glib2 to enable autofree'ing of stack variables exiting scope
mbox series

Message ID 20190725084341.8287-1-berrange@redhat.com
Headers show
Series
  • require newer glib2 to enable autofree'ing of stack variables exiting scope
Related show

Message

Daniel P. Berrangé July 25, 2019, 8:43 a.m. UTC
Both GCC and CLang support a C extension attribute((cleanup)) which
allows you to define a function that is invoked when a stack variable
exits scope. This typically used to free the memory allocated to it,
though you're not restricted to this. For example it could be used to
unlock a mutex.

We could use that functionality now, but the syntax is a bit ugly in
plain C. Since version 2.44 of GLib, there have been a few macros to
make it more friendly to use - g_autofree, g_autoptr and
G_DEFINE_AUTOPTR_CLEANUP_FUNC.

  https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html

  https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/

The key selling point is that it simplifies the cleanup code paths,
often eliminating the need to goto cleanup labels. This improves
the readability of the code and makes it less likely that you'll
leak memory accidentally.

Inspired by seeing it added to glib, and used in systemd, Libvirt
finally got around to adopting this in Feb 2019. Overall our
experience with it has been favourable/positive, with the code
simplification being very nice.

The main caveats with it are

 - Only works with GCC or CLang. We don't care as those are
   the only two compilers we declare support for.

 - You must always initialize the variables when declared
   to ensure predictable behaviour when they leave scope.
   Chances are most methods with goto jumps for cleanup
   are doing this already

 - You must not directly return the value that's assigned
   to a auto-cleaned variable. You must steal the pointer
   in some way. ie

    BAD:
        g_autofree char *wibble = g_strdup("wibble")
	....
	return wibble;

    GOOD:
        g_autofree char *wibble = g_strdup("wibble")
	...
	return g_steal_pointer(wibble);

    g_steal_pointer is an inline function which simply copies
    the pointer to a new variable, and sets the original variable
    to NULL, thus avoiding cleanup.

I've illustrated the usage by converting a bunch of the crypto code in
QEMU to use auto cleanup.

Changed on v2:

 - Actually commit the rest of the changes to patch 3 so that what's
   posted works :-) Sigh.

Daniel P. Berrangé (3):
  glib: bump min required glib library version to 2.48
  crypto: define cleanup functions for use with g_autoptr
  crypto: use auto cleanup for many stack variables

 configure                   |  2 +-
 crypto/afsplit.c            | 28 ++++----------
 crypto/block-luks.c         | 74 +++++++++++--------------------------
 crypto/block.c              | 15 +++-----
 crypto/hmac-glib.c          |  5 ---
 crypto/pbkdf.c              |  5 +--
 crypto/secret.c             | 38 ++++++++-----------
 crypto/tlscredsanon.c       | 16 +++-----
 crypto/tlscredspsk.c        |  5 +--
 crypto/tlscredsx509.c       | 16 +++-----
 include/crypto/block.h      |  2 +
 include/crypto/cipher.h     |  2 +
 include/crypto/hmac.h       |  2 +
 include/crypto/ivgen.h      |  2 +
 include/crypto/tlssession.h |  2 +
 include/glib-compat.h       | 42 +--------------------
 16 files changed, 78 insertions(+), 178 deletions(-)

Comments

Marc-André Lureau July 31, 2019, 12:59 p.m. UTC | #1
On Thu, Jul 25, 2019 at 12:44 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> Both GCC and CLang support a C extension attribute((cleanup)) which
> allows you to define a function that is invoked when a stack variable
> exits scope. This typically used to free the memory allocated to it,
> though you're not restricted to this. For example it could be used to
> unlock a mutex.
>
> We could use that functionality now, but the syntax is a bit ugly in
> plain C. Since version 2.44 of GLib, there have been a few macros to
> make it more friendly to use - g_autofree, g_autoptr and
> G_DEFINE_AUTOPTR_CLEANUP_FUNC.
>
>   https://developer.gnome.org/glib/stable/glib-Miscellaneous-Macros.html
>
>   https://blogs.gnome.org/desrt/2015/01/30/g_autoptr/
>
> The key selling point is that it simplifies the cleanup code paths,
> often eliminating the need to goto cleanup labels. This improves
> the readability of the code and makes it less likely that you'll
> leak memory accidentally.
>
> Inspired by seeing it added to glib, and used in systemd, Libvirt
> finally got around to adopting this in Feb 2019. Overall our
> experience with it has been favourable/positive, with the code
> simplification being very nice.
>
> The main caveats with it are
>
>  - Only works with GCC or CLang. We don't care as those are
>    the only two compilers we declare support for.
>
>  - You must always initialize the variables when declared
>    to ensure predictable behaviour when they leave scope.
>    Chances are most methods with goto jumps for cleanup
>    are doing this already
>
>  - You must not directly return the value that's assigned
>    to a auto-cleaned variable. You must steal the pointer
>    in some way. ie
>
>     BAD:
>         g_autofree char *wibble = g_strdup("wibble")
>         ....
>         return wibble;
>
>     GOOD:
>         g_autofree char *wibble = g_strdup("wibble")
>         ...
>         return g_steal_pointer(wibble);
>
>     g_steal_pointer is an inline function which simply copies
>     the pointer to a new variable, and sets the original variable
>     to NULL, thus avoiding cleanup.
>
> I've illustrated the usage by converting a bunch of the crypto code in
> QEMU to use auto cleanup.
>
> Changed on v2:
>
>  - Actually commit the rest of the changes to patch 3 so that what's
>    posted works :-) Sigh.
>
> Daniel P. Berrangé (3):
>   glib: bump min required glib library version to 2.48
>   crypto: define cleanup functions for use with g_autoptr
>   crypto: use auto cleanup for many stack variables

Series:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

>
>  configure                   |  2 +-
>  crypto/afsplit.c            | 28 ++++----------
>  crypto/block-luks.c         | 74 +++++++++++--------------------------
>  crypto/block.c              | 15 +++-----
>  crypto/hmac-glib.c          |  5 ---
>  crypto/pbkdf.c              |  5 +--
>  crypto/secret.c             | 38 ++++++++-----------
>  crypto/tlscredsanon.c       | 16 +++-----
>  crypto/tlscredspsk.c        |  5 +--
>  crypto/tlscredsx509.c       | 16 +++-----
>  include/crypto/block.h      |  2 +
>  include/crypto/cipher.h     |  2 +
>  include/crypto/hmac.h       |  2 +
>  include/crypto/ivgen.h      |  2 +
>  include/crypto/tlssession.h |  2 +
>  include/glib-compat.h       | 42 +--------------------
>  16 files changed, 78 insertions(+), 178 deletions(-)
>
> --
> 2.21.0
>
>
Alex Bennée July 31, 2019, 2:04 p.m. UTC | #2
Daniel P. Berrangé <berrange@redhat.com> writes:

> Both GCC and CLang support a C extension attribute((cleanup)) which
> allows you to define a function that is invoked when a stack variable
> exits scope. This typically used to free the memory allocated to it,
> though you're not restricted to this. For example it could be used to
> unlock a mutex.
<snip>
>
>     GOOD:
>         g_autofree char *wibble = g_strdup("wibble")
> 	...
> 	return g_steal_pointer(wibble);
>
>     g_steal_pointer is an inline function which simply copies
>     the pointer to a new variable, and sets the original variable
>     to NULL, thus avoiding cleanup.

Surely this is a particular use case where you wouldn't use g_autofree
to declare the variable as you intending to return it to the outer scope?

--
Alex Bennée
Eric Blake July 31, 2019, 2:08 p.m. UTC | #3
On 7/31/19 9:04 AM, Alex Bennée wrote:
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
>> Both GCC and CLang support a C extension attribute((cleanup)) which
>> allows you to define a function that is invoked when a stack variable
>> exits scope. This typically used to free the memory allocated to it,
>> though you're not restricted to this. For example it could be used to
>> unlock a mutex.
> <snip>
>>
>>     GOOD:
>>         g_autofree char *wibble = g_strdup("wibble")
>> 	...
>> 	return g_steal_pointer(wibble);
>>
>>     g_steal_pointer is an inline function which simply copies
>>     the pointer to a new variable, and sets the original variable
>>     to NULL, thus avoiding cleanup.
> 
> Surely this is a particular use case where you wouldn't use g_autofree
> to declare the variable as you intending to return it to the outer scope?

Actually, it's still quite useful if you have intermediate returns:

g_autofree char *wibble = NULL;
if (something)
  return NULL;
wibble = g_strdup("wibble");
if (something_else)
  return NULL;
return g_steal_pointer(wibble);
Daniel P. Berrangé July 31, 2019, 2:10 p.m. UTC | #4
On Wed, Jul 31, 2019 at 03:04:29PM +0100, Alex Bennée wrote:
> 
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > Both GCC and CLang support a C extension attribute((cleanup)) which
> > allows you to define a function that is invoked when a stack variable
> > exits scope. This typically used to free the memory allocated to it,
> > though you're not restricted to this. For example it could be used to
> > unlock a mutex.
> <snip>
> >
> >     GOOD:
> >         g_autofree char *wibble = g_strdup("wibble")
> > 	...
> > 	return g_steal_pointer(wibble);
> >
> >     g_steal_pointer is an inline function which simply copies
> >     the pointer to a new variable, and sets the original variable
> >     to NULL, thus avoiding cleanup.
> 
> Surely this is a particular use case where you wouldn't use g_autofree
> to declare the variable as you intending to return it to the outer scope?

I think it depends on the situation. Obviously real code will have
something in the "..." part I snipped.

You have 20 code paths that can result in returning with an error, where
you want to have all variables freed, and only 1 code path for success
Then it makes sense to use g_autofree + g_steal_pointer to eliminate
many goto jumps.

If you have only 1 error path and 1 success path, then a traditional
g_free() call is may well be sufficient.

IOW, as with many coding "rules", there's scope to use personal
judgement as to when it is right to ignore it vs folow it.

Regards,
Daniel
Alex Bennée July 31, 2019, 2:33 p.m. UTC | #5
Daniel P. Berrangé <berrange@redhat.com> writes:

> On Wed, Jul 31, 2019 at 03:04:29PM +0100, Alex Bennée wrote:
>>
>> Daniel P. Berrangé <berrange@redhat.com> writes:
>>
>> > Both GCC and CLang support a C extension attribute((cleanup)) which
>> > allows you to define a function that is invoked when a stack variable
>> > exits scope. This typically used to free the memory allocated to it,
>> > though you're not restricted to this. For example it could be used to
>> > unlock a mutex.
>> <snip>
>> >
>> >     GOOD:
>> >         g_autofree char *wibble = g_strdup("wibble")
>> > 	...
>> > 	return g_steal_pointer(wibble);
>> >
>> >     g_steal_pointer is an inline function which simply copies
>> >     the pointer to a new variable, and sets the original variable
>> >     to NULL, thus avoiding cleanup.
>>
>> Surely this is a particular use case where you wouldn't use g_autofree
>> to declare the variable as you intending to return it to the outer scope?
>
> I think it depends on the situation. Obviously real code will have
> something in the "..." part I snipped.
>
> You have 20 code paths that can result in returning with an error, where
> you want to have all variables freed, and only 1 code path for success
> Then it makes sense to use g_autofree + g_steal_pointer to eliminate
> many goto jumps.
>
> If you have only 1 error path and 1 success path, then a traditional
> g_free() call is may well be sufficient.

I suspect this would be worth a write up in HACKING or CODING_STYLE with
the next iteration? (which reminds me we should really merge and .rst up
those documents)

>
> IOW, as with many coding "rules", there's scope to use personal
> judgement as to when it is right to ignore it vs folow it.
>
> Regards,
> Daniel


--
Alex Bennée