Re: [PATCH BlueZ 1/7] android: Use 16-bit UUID for SDP search

[Szymon Janc <szymon.janc@tieto.com>]

Hi Anderson,

On Wednesday 12 of February 2014 07:28:24 Anderson Lizardo wrote:
> Hi Szymon,
> 
> On Wed, Feb 12, 2014 at 6:59 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> >> Applied all except 6/7, I think we should probably return an error if
> >> there is an attempt to register a service out of range, then the
> >> caller can assert so we can have a proper test for it that expect an
> >> error in such case.
> > 
> > Well, currently IPC depends on hal-msg.h which defines max allowed service
> > ID and using something bigger is a code bug. We could have make IPC
> > independent of hal-msg.h and just verify registered ID in runtime but
> > that would add extra code for each caller with no profit as IDs are fixed
> > anyway.
> 
> Personally, I don't have strong opinions on using assert() versus
> raise(SIGTERM) (which is how runtime error conditions seem to be
> handled). Initially I went with raise(SIGTERM), but then I noticed the
> IDs are statically defined, and there is no way to give a invalid ID
> to ipc_register(),  unless due to programming error (for which
> assert() is ideal, due to low overhead and no introduction of dead
> code).
> 
> That said, this patch is not critical, but only a check so future
> users of ipc_register() don't reintroduce a similar crash fixed by the
> other commit.
> 
> > That test fix is invalid as we actually want to test if IPC handles
> > out-of-
> > bound service ID correctly (when receiving register command).
> > And I'm not sure why this actually was causing any problems since that
> > test is not registering handlers for out-of-bound service ID, just sends
> > ipc message with such ID.
> 
> I forgot to clarify this on the commit message: the "out of bound"
> service ID is still passed on the IPC message. What I fixed is the
> service ID field used solely for registering the handlers (i.e. passed
> to ipc_register()). If you check the changed struct, there is another
> field for the IPC headers where there is still the expected (out of
> bound) ID.
> 
> In the current format, ipc_register() must not receive an "out of
> bound" ID otherwise memory corruptions occur, which introduces subtle
> bugs (in my case the crash happened in very specific compilation
> parameters and valgrind didn't help because the corrupted structures
> were static).

Yes, I was looking at service ID in wrong line. This seems ok now.

-- 
BR
Szymon Janc