Re: [PATCH] avrcp: Fix wrong pointer check

[Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com>]

On Mon, Feb 03, 2014 at 01:20:11AM -0800, Luiz Augusto von Dentz wrote:
> Hi Andrei,
> 
> On Sun, Feb 2, 2014 at 11:51 PM, Andrei Emeltchenko
> <Andrei.Emeltchenko.news@gmail.com> wrote:
> > Hi Luiz,
> >
> > On Sun, Feb 02, 2014 at 08:03:34AM -0800, Luiz Augusto von Dentz wrote:
> >> Hi Andrei,
> >>
> >> On Fri, Jan 31, 2014 at 12:33 AM, Andrei Emeltchenko
> >> <Andrei.Emeltchenko.news@gmail.com> wrote:
> >> > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>
> >> >
> >> > There is wrong assumption that handler might be NULL while it is a
> >> > pointer to a struct table so check instead for struct members. This
> >> > fixes accessing wrong memory.
> >> > ---
> >> >  profiles/audio/avrcp.c | 4 ++--
> >> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> >> > index df88138..5030ce1 100644
> >> > --- a/profiles/audio/avrcp.c
> >> > +++ b/profiles/audio/avrcp.c
> >> > @@ -1673,7 +1673,7 @@ static size_t handle_vendordep_pdu(struct avctp *conn, uint8_t transaction,
> >> >                         break;
> >> >         }
> >> >
> >> > -       if (!handler || handler->code != *code) {
> >> > +       if (!handler->code || handler->code != *code) {
> >>
> >> The code checks if  session->control_handlers is initialized and Im
> >> not sure what is the invalid memory access you are talking about since
> >> handle->code is no a pointer, I do agree that we should probably drop
> >> the second check for the handler in the lines bellow.
> >
> > handler is a pointer, and it points to
> >
> > static const struct control_pdu_handler control_handlers[]
> > table with the last element:
> >
> > ...
> >                 { },
> > };
> > ...
> >
> > So checking for !handler is pointless.
> 
> Right, because checking for pointer is pointless, yes Im being
> sarcastic here... Now lets be clear, you are changing a check of a
> pointer to a value and claiming it fixes invalid accesses which does
> not make any sense, what could make sense is to check if
> handler->pdu_id == pdu->pdu_id since that what we check when we lookup
> for a handle.

No, we break if handler->pdu_id == pdu->pdu_id one line above ;)

Best regards 
Andrei Emeltchenko 

> 
> 
> 
> -- 
> Luiz Augusto von Dentz