Re: [PATCH obexd 2/2] gobex: Fix ABORT request not processing

[Hendrik Sattler <post@hendrik-sattler.de>]

Am 13.01.2012 12:21, schrieb Johan Hedberg:
> Hi Hendrik,
>
> On Fri, Jan 13, 2012, Hendrik Sattler wrote:
>>>>> 		opcode = obex->rx_last_op;
>>>>> 		/* Unexpected response -- fail silently */
>>>>>-		if (opcode > 0x1f && opcode < 0xff) {
>>>>>+		if (opcode > G_OBEX_OP_ABORT && opcode < 0xff) {
>>>>> 			obex->rx_data = 0;
>>>>> 			return TRUE;
>>>>> 		}
>>>>
>>>>This one always evaluates to false because 0xff == G_OBEX_OP_ABORT 
>>>> |
>>>>FINAL_BIT.
>>>
>>> The value of obex->rx_last_op is stored with the final bit cleared,
>>> so the < 0xff part could also be removed from the test. The value
>>> 0x1f has been picked because the IrDA OBEX specification defines 
>>> 0x10
>>> - 0x1f as a user-definable range, so anything between 0x1f and 0x7f
>>> isn't actually valid. The correct test would then become:
>>>
>>>	if (opcode > 0x1f && opcode != G_OBEX_OP_ABORT)
>>>
>>> Jaganath, with the above change the patch should be ok, but the 
>>> more
>>> surprising thing to me here is that this implies we're missing one 
>>> or
>>> more unit tests for Abort. Could you create a patch to add those 
>>> too?
>>> (if we're really strict those tests should go in before this patch 
>>> so
>>> that it's actually possible to see that the patch makes a
>>> difference).
>>
>> User-definable does not mean invalid. It only means that these can 
>> be
>> used for custom commands. These are still bound to the rules of the
>> OBEX protocol. I've never seen one using that, though.
>
> Exactly. You might want to re-read what I said and the test I 
> proposed.

I did.

>> Else you'd also have to filter 0x04 and the range 0x08-0x0f because
>> these are marked as "reserved".
>
> With the test I proposed we filter neither reserved (since they might
> get meaning in new spec versions) nor user-defined opcodes.

Your propose if-line discards all packets with opcodes in the range
0x20-0x1e. I would remove that if-line (and also the wrong comments 
that
talks about responses but means requests).

OTOH, the reserved and user-defined packets will fail immediately after
that anyway due to the check of the length of non-header data returning
-1 for those cases.
Not only that but it immediately drops connection because of that? It 
could
at least get the packet and send a negative response.

HS