Re: [PATCH 2/3] Fix avrcp unregister potential crash

[Johan Hedberg <johan.hedberg@gmail.com>]

Hi,

On Fri, May 18, 2012, Johan Hedberg wrote:
> On Fri, May 18, 2012, Lucas De Marchi wrote:
> > >> --- a/audio/avrcp.c
> > >> +++ b/audio/avrcp.c
> > >> @@ -1272,8 +1272,11 @@ void avrcp_unregister(const bdaddr_t *src)
> > >>
> > >>       servers = g_slist_remove(servers, server);
> > >>
> > >> -     remove_record_from_server(server->ct_record_id);
> > >> -     remove_record_from_server(server->tg_record_id);
> > >> +     if (server->ct_record_id)
> > >> +             remove_record_from_server(server->ct_record_id);
> > >> +
> > >> +     if (server->tg_record_id)
> > >> +             remove_record_from_server(server->tg_record_id);
> > >>
> > >>       avctp_unregister(&server->src);
> > >>       g_free(server);
> > >
> > > I don't think the commit message for this patch is truthful. If you look
> > > at the code the remove_record_from_server will return ENOENT if you pass
> > > it a non-existent handle. I.e. it will not crash. Please fix the commit
> > > message to reflect what exactly is being fixed (i.e. an unnecessary call
> > > to remove_record_from_server if there is no record handle).
> > 
> > Then I don't see a reason: just let remove_record_from_server() fail ?
> > 
> > Chan, what are you fixing here?
> 
> As discussed on IRC (and Chanyeol should really have explained this in
> the commit message) handle 0 is actually the SDP server's own "master"
> record. So it is plausible that weird behavior might occur if trying to
> remove it. Since this is a SDP server internal detail and it doesn't
> really make sense to remove it I'm thinking that
> remove_record_from_server should return EINVAL if passed a 0 value.

I already pushed a patch for this so no need to work on it anymore.

Johan