[1/3] media: atomisp: revert "don't pass a pointer to a local variable"

The gcc is warning about returning a pointer to a local variable
is a false positive.

The type of handle is "struct ia_css_rmgr_vbuf_handle **" and
"h.vptr" is left to NULL, so the "if ((*handle)->vptr == 0x0)"
check always succeeds when the "*handle = &h;" statement which
gcc warns about executes. Leading to this statement being executed:

	rmgr_pop_handle(pool, handle);

If that succeeds,  then *handle has been set to point to one of
the pre-allocated array of handles, so it no longer points to h.

If that fails the following statement will be executed:

	/* Note that handle will change to an internally maintained one */
	ia_css_rmgr_refcount_retain_vbuf(handle);

Which allocated a new handle from the array of pre-allocated handles
and then makes *handle point to this. So the address of h is actually
never returned.

The fix for the false-postive compiler warning actually breaks the code,
the new:

	**handle = h;

is part of a "if (pool->copy_on_write) { ... }" which means that the
handle where *handle points to should be treated read-only, IOW
**handle must never be set, instead *handle must be set to point to
a new handle (with a copy of the contents of the old handle).

The old code correctly did this and the new fixed code gets this wrong.

Note there is another patch in this series, which fixes the warning
in another way.

Fixes: fa1451374ebf ("media: atomisp: don't pass a pointer to a local variable")
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../staging/media/atomisp/pci/runtime/rmgr/src/rmgr_vbuf.c    | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
  

On Sun, Jun 12, 2022 at 6:06 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The gcc is warning about returning a pointer to a local variable
> is a false positive.
>
> The type of handle is "struct ia_css_rmgr_vbuf_handle **" and
> "h.vptr" is left to NULL, so the "if ((*handle)->vptr == 0x0)"
> check always succeeds when the "*handle = &h;" statement which
> gcc warns about executes. Leading to this statement being executed:
>
>         rmgr_pop_handle(pool, handle);
>
> If that succeeds,  then *handle has been set to point to one of
> the pre-allocated array of handles, so it no longer points to h.
>
> If that fails the following statement will be executed:
>
>         /* Note that handle will change to an internally maintained one */
>         ia_css_rmgr_refcount_retain_vbuf(handle);
>
> Which allocated a new handle from the array of pre-allocated handles
> and then makes *handle point to this. So the address of h is actually
> never returned.
>
> The fix for the false-postive compiler warning actually breaks the code,
> the new:
>
>         **handle = h;
>
> is part of a "if (pool->copy_on_write) { ... }" which means that the
> handle where *handle points to should be treated read-only, IOW
> **handle must never be set, instead *handle must be set to point to
> a new handle (with a copy of the contents of the old handle).
>
> The old code correctly did this and the new fixed code gets this wrong.
>
> Note there is another patch in this series, which fixes the warning
> in another way.

> Fixes: fa1451374ebf ("media: atomisp: don't pass a pointer to a local variable")

Dunno for media subsystem, but for ones that Greg is maintain, the
point is that revert itself is already kinda fix and no need to have a
Fixes tag, instead the commit message should clearly have the
automatically generated line of revert (with the rest of the
explanation why that is needed). Just sharing my experience.
  

On Sun, Jun 12, 2022 at 09:22:55PM +0200, Andy Shevchenko wrote:
> > Note there is another patch in this series, which fixes the warning
> > in another way.
> 
> > Fixes: fa1451374ebf ("media: atomisp: don't pass a pointer to a local variable")
> 
> Dunno for media subsystem, but for ones that Greg is maintain, the
> point is that revert itself is already kinda fix and no need to have a
> Fixes tag, instead the commit message should clearly have the
> automatically generated line of revert (with the rest of the
> explanation why that is needed). Just sharing my experience.

How would that work in this case?  We don't have a reference to the git
hash.

The `git revert` command came from early days of git and I always
feel like it hasn't keep up with how git is used these days.  The
subject doesn't have the subsystem prefix.  The commit message is wrong.
It uses the full git hash instead of the 12 char hash.  It doesn't have
a fixes tag.  Hans's commit is only correct because he re-wrote
basically everything.

Do a `git --grep=revert`.  Some of them you can grep for "This reverts
commit 8bdc2a190105e862dfe7a4033f2fd385b7e58ae8." but there are a lot
which are not machine parsable like:

bd06db5ff9af ("lib/flex_proportions.c: remove local_irq_ops in fprop_new_period()")
4af2bd190a5b ("Revert "squashfs: provide backing_dev_info in order to disable read-ahead"")
646728dff254 ("dmaengine: Revert "dmaengine: add verification of DMA_INTERRUPT capability for dmatest"")

I feel like we should encourage people to not use git revert because
otherwise we're kind of setting them up for failure.

regards,
dan carpenter
  

On Mon, Jun 13, 2022 at 05:58:20PM +0300, Dan Carpenter wrote:
> On Sun, Jun 12, 2022 at 09:22:55PM +0200, Andy Shevchenko wrote:
> > > Note there is another patch in this series, which fixes the warning
> > > in another way.
> > 
> > > Fixes: fa1451374ebf ("media: atomisp: don't pass a pointer to a local variable")
> > 
> > Dunno for media subsystem, but for ones that Greg is maintain, the
> > point is that revert itself is already kinda fix and no need to have a
> > Fixes tag, instead the commit message should clearly have the
> > automatically generated line of revert (with the rest of the
> > explanation why that is needed). Just sharing my experience.
> 
> How would that work in this case?  We don't have a reference to the git
> hash.

What do you mean? `git revert` adds the hash of the commit being reverted.

> The `git revert` command came from early days of git and I always
> feel like it hasn't keep up with how git is used these days.  The
> subject doesn't have the subsystem prefix.  The commit message is wrong.
> It uses the full git hash instead of the 12 char hash.  It doesn't have
> a fixes tag.  Hans's commit is only correct because he re-wrote
> basically everything.
> 
> Do a `git --grep=revert`.  Some of them you can grep for "This reverts
> commit 8bdc2a190105e862dfe7a4033f2fd385b7e58ae8." but there are a lot
> which are not machine parsable

Why not? The format of the string hasn't been changed, no difference from other
patterns.

P.S. I have told this as my experience and what Greg told me, feel free to
discuss with him or others, I'm pretty much okay if Hans' patch goes as is.
  

On Mon, Jun 13, 2022 at 06:39:19PM +0300, Andy Shevchenko wrote:
> > Do a `git --grep=revert`.  Some of them you can grep for "This reverts
> > commit 8bdc2a190105e862dfe7a4033f2fd385b7e58ae8." but there are a lot
> > which are not machine parsable
> 
> Why not? The format of the string hasn't been changed, no difference from other
> patterns.
> 

With the Fixes tag you can just do a:

	git log | grep Fixes: | cut -d : -f 2 | cut -d '(' -f 1

It's easily machine parseable.  But if you look at the examples I posted
they're stuff like this:

    This reverts commit 9eec1d897139e5d ("squashfs: provide backing_dev_info

It can't be grepped for, it needs a human to try figure it out.  And the
reason for that is that we always tell people that git hashes need to
be in a specific format which git revert violates.

Having two hashes *is* duplicative but if we're to delete a hash we
should do Hans did and delete the "This reverts commit
fb561bf9abde49f7e00fdbf9ed2ccf2d86cac8ee." line.  (As an aside, in
that commit the reverts line is not a Fixes line.  The original commit
was a temporary hack and it was deleted when it was no longer required.
So reverts and Fixes are not the same.  Reverts is ambiguous.)

The problem with the reverts line is that most other people besides Greg
only look for the Fixes tags.  It had never occured to me to look for
the reverts line.  I was just reading an LWN article about bugs in
-stable and LWN only used Fixes tags, not reverts lines.  Or when people
are backporting patches I tell them to look for the Fixes tags to see if
they are backporting buggy patches.  If they're searching
lore.kernel.org most people will use the 12 char git hash instead of the
full hash.

My main problem with `git revert` is that it writes the commit message
for you and it does it really badly.  When I'm reviewing those patches
I have to tell people, "No, never use git revert format.  Send normal
patches."  I always tell them to redo it like Hans did.

Subject is wrong:
https://lore.kernel.org/all/20220614011528.32118-1-tangmeng@uniontech.com/

No Signed-off-by:
https://lore.kernel.org/all/BN9PR12MB5257FB6CA192626D5D108C2DFCAB9@BN9PR12MB5257.namprd12.prod.outlook.com/

Terrible commit message:
https://lore.kernel.org/all/20210414233533.24012-2-qingqing.zhuo@amd.com/

No commit message.
https://lore.kernel.org/all/20220613132116.2021055-2-idosch@nvidia.com/

These are just the first view I looked at from yesterday afternoon.
Almost every patch with Revert in the subject needs to be NAKed.

regards,
dan carpenter