Re: [PATCH v4 1/3] block/rbd: encryption nit fixes

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, Jan 12, 2023 at 06:07:58PM +0100, Ilya Dryomov wrote:
> On Thu, Jan 12, 2023 at 3:46 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > On Thu, Jan 12, 2023 at 03:26:56PM +0100, Ilya Dryomov wrote:
> > > On Thu, Jan 12, 2023 at 1:35 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> > > >
> > > > On Sun, Nov 20, 2022 at 04:28:34AM -0600, Or Ozeri wrote:
> > > > > Add const modifier to passphrases,
> > > > > and remove redundant stack variable passphrase_len.
> > > > >
> > > > > Signed-off-by: Or Ozeri <oro@il.ibm.com>
> > > > > ---
> > > > >  block/rbd.c | 24 ++++++++++--------------
> > > > >  1 file changed, 10 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/block/rbd.c b/block/rbd.c
> > > > > index f826410f40..e575105e6d 100644
> > > > > --- a/block/rbd.c
> > > > > +++ b/block/rbd.c
> > > > > @@ -330,7 +330,7 @@ static int qemu_rbd_set_keypairs(rados_t cluster, const char *keypairs_json,
> > > > >  #ifdef LIBRBD_SUPPORTS_ENCRYPTION
> > > > >  static int qemu_rbd_convert_luks_options(
> > > > >          RbdEncryptionOptionsLUKSBase *luks_opts,
> > > > > -        char **passphrase,
> > > > > +        const char **passphrase,
> > > > >          size_t *passphrase_len,
> > > > >          Error **errp)
> > > > >  {
> > > > > @@ -341,7 +341,7 @@ static int qemu_rbd_convert_luks_options(
> > > > >  static int qemu_rbd_convert_luks_create_options(
> > > > >          RbdEncryptionCreateOptionsLUKSBase *luks_opts,
> > > > >          rbd_encryption_algorithm_t *alg,
> > > > > -        char **passphrase,
> > > > > +        const char **passphrase,
> > > > >          size_t *passphrase_len,
> > > > >          Error **errp)
> > > > >  {
> > > > > @@ -384,8 +384,7 @@ static int qemu_rbd_encryption_format(rbd_image_t image,
> > > > >                                        Error **errp)
> > > > >  {
> > > > >      int r = 0;
> > > > > -    g_autofree char *passphrase = NULL;
> > > > > -    size_t passphrase_len;
> > > > > +    g_autofree const char *passphrase = NULL;
> > > >
> > > > This looks wierd.  If it is as const string, why are
> > > > we free'ing it ?  Either want g_autofree, or const,
> > > > but not both.
> > >
> > > Just curious, is it a requirement imposed by g_autofree?  Otherwise
> > > pointer constness and pointee lifetime are completely orthogonal and
> > > freeing (or, in this case, wanting to auto-free) an object referred to
> > > by a const pointer seems perfectly fine to me.
> >
> > Free'ing a const point is not OK
> >
> > $ cat c.c
> > #include <stdlib.h>
> > void bar(const char *foo) {
> >     free(foo);
> > }
> >
> > $ gcc -Wall -c c.c
> > c.c: In function ‘bar’:
> > c.c:5:10: warning: passing argument 1 of ‘free’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
> >     5 |     free(foo);
> >       |          ^~~
> > In file included from c.c:2:
> > /usr/include/stdlib.h:568:25: note: expected ‘void *’ but argument is of type ‘const char *’
> >   568 | extern void free (void *__ptr) __THROW;
> >       |                   ~~~~~~^~~~~
> >
> > The g_autofree happens to end up hiding this warning, because the const
> > annotation isn't propagated to the registere callback, but that doesn't
> > mean we should do that.
> >
> > When a programmer sees a variable annotated const, they expect that
> > either someone else is responsible for free'ing it, or that the data
> > is statically initialized or stack allocated and thus doesn't need
> > free'ing.  So g_autofree + const is just wrong.
> 
> FWIW many believe that this specification of free() was a mistake and
> that it should have been specified to take const void *.  Some projects
> actually went ahead and fixed that: kfree() and friends in the Linux
> kernel take const void *, for example.  C++ delete operator works on
> const pointers as well -- because object creation and destruction is
> fundamentally independent of modification.

I'd really not like that as IMHO seeing the 'const' gives an important
hint to developers as to who is responsible for the releasing the pointer

> But this is more of a philosophical thing...  I asked about g_autofree
> because a quick grep revealed a bunch of g_autofree const char * locals
> in the tree.  Or would probably prefer to just drop const here ;)

IMHO those existing cases are all bugs that we should fix, along with
adding a rule to checkpatch.pl to detect this mistake.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|