On Sun, Jun 12, 2022 at 6:06 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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.
--
With Best Regards,
Andy Shevchenko
