Hi Szymon, On Wed, Feb 12, 2014 at 6:59 AM, Szymon Janc <szymon.janc@xxxxxxxxx> 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). Best Regards, -- Anderson Lizardo http://www.indt.org/?lang=en INdT - Manaus - Brazil -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
