Hi Luiz,
On Tue, Dec 3, 2013 at 7:40 AM, Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
> Hi Anderson,
>
> On Tue, Dec 3, 2013 at 1:24 PM, Anderson Lizardo
> <anderson.lizardo@xxxxxxxxxxxxx> wrote:
>> Hi Luiz,
>>
>> On Tue, Dec 3, 2013 at 5:36 AM, Luiz Augusto von Dentz
>> <luiz.dentz@xxxxxxxxx> wrote:
>>> --- a/profiles/audio/avctp.c
>>> +++ b/profiles/audio/avctp.c
>>> @@ -1488,7 +1488,7 @@ static struct avctp_pending_req *pending_create(struct avctp_channel *chan,
>>> tmp = g_slist_copy(chan->processed);
>>>
>>> /* Find first unused transaction id */
>>> - for (l = tmp; l; l = l->next) {
>>> + for (l = tmp; l; l = g_slist_next(l)) {
>>
>> Are you sure this fixes the problem? AFAIK g_list_next() will still
>> access invalid memory unless the "next" pointer is saved *before* the
>> current entry is freed. See e.g. remove_temp_devices() in
>> src/adapter.c.
>
> Yep, I tested and it fixes the problem. The problem is not actually
> removing the item, but reassigning tmp to the head of the list which
> can be NULL so doing l = l->next before deleting the node doesn't help
> in this case. Anyway this is now applied but we will be changing the
> logic how to check the available transaction to use bitmask operations
> so we find the next transaction without looping in a list.
In this case I agree that it is not the same situation as I described.
I was looking only at the patch itself, not the original code :/ In
any case, good to know you are going to simplify it.
BTW, I noticed that the "chan->transaction %= 16;" in that same
function (in two places) is unnecessary, because chan->transaction is
"uint8_t transaction:4" (4-bit bitfield) so the post increment
operation before this statement will always overflow "transaction" and
it will go back to 0 once it reaches 15.
Best Regards,
--
Anderson Lizardo
Instituto Nokia de Tecnologia - 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
