[PATCH 1/3] fix code generation for emit_size_read() to check for buffer overflow

[Aleksander Morgado]

On Tue, Oct 7, 2014 at 1:41 PM, Aleksander Morgado
<aleksander at aleksander.es> wrote:
> On Tue, Oct 7, 2014 at 12:34 PM, Aleksander Morgado
> <aleksander at aleksander.es> wrote:
>> emit_size_read() is only used during validate(), and therefore we can
>> safely add code to return FALSE (indicating message invalid) at any
>> point. So, instead of going on with the parsing, if we detect that the
>> we don't have enough bytes in the buffer to read the string/array
>> size-variable, we can just g_warning() and return an error.
>>
>> See attached, patch; what do you think?
>
> Forget about the previous patch; this one looks better.
>
> Plus, I added a unit test to reproduce the issue. git master segfaults
> on the unit test, as it tries to read a guint16 from a buffer of 0
> bytes; while with the patch on we just get a warning and fully ignore
> the invalid TLV.


Some more context, that I just commented in IRC, and not sure Thomas got...

The code doesn't return a GError in validate(), just does a g_warning,
because we don't want to halt the full message parsing if we detect a
TLV with wrong contents. An incorrect TLV, or just a TLV which we
don't know how to parse, shouldn't make the parsing of other TLVs
break. validate() in this case is done for each TLV while we parse the
received message; and we do an initial parsing of the whole message by
default.

Another, maybe better, approach would be to only parse those TLVs that
the user wants. i.e. the 'output' bundle would only fill in the parsed
fields when a given TLV is requested. And in that case, if a single
TLV is requested from the output bundle, we could then run validate()
and return a GError. IIRC this is more or less what I did in libmbim
actually, where the result of a client operation is not an already
parsed output bundle, but the actual message itself, and in that case
only the user-requested TLVs get parsed and built.

I'd suggest to leave the logic as is for now (for these bugs I mean),
but target the change for the next release. That change will not only
allow us to return errors if parsing a given TLV fails; it will also
end up speeding the logic a bit likely, as we won't be parsing the
full message always. even if the user doesn't want any TLV. Plus, I
don't think we would need any API change in libqmi-glib.

-- 
Aleksander
https://aleksander.es