Re: [PATCH 04/20] dbus: Fix _dbus_kernel_calculate_bloom for multiple arguments.

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5817 bytes --]

Hi Andrew,

On 03/14/2016 05:11 PM, Andrzej Zaborowski wrote:
>   Hi Denis,
>
> On 14 March 2016 at 17:49, Denis Kenzior <denkenz@gmail.com> wrote:
>> Hi Andrew,
>>
>> On 03/13/2016 10:41 PM, Andrew Zaborowski wrote:
>>>
>>> It seems that l_dbus_message_iter_get_type can't be used to get the
>>> value of a single argument and be called repeatedly, it might crash if
>>
>>
>> Looking at l_dbus_message_iter_get_type, it just returns
>> iter->sig_start[iter->sig_pos].  Is there something deeper going on here
>> that is not readily apparent?
>
> Sorry, l_dbus_message_iter_get_type is a typo, I wanted to say
> message_iter_next_entry here.  This function calls it with a fixed
> number of parameters and sooner or later it will receive a message
> with more arguments and by my reading will segfault.
>

So l_dbus_message_iter_next_entry is assuming that the programmer knows 
the signature of the next parameter and passes in a variable meant to 
hold that parameter.  If there's a mismatch, then yes, it might crash. 
However, that is why we check the signature ahead of time.

I guess I'm still not quite sure what you're trying to say here :)

>>
>>> the number and types of call arguments don't match the message arguments.
>>> Also try to support the string arguments that come after non-string
>>> arguments in the bloom filter.
>>
>>
>>  From my reading of systemd code, I didn't think this was possible.  But I
>> could be wrong...
>
> I see no problem with the bloom filter containing, say, arg3="hello",
> even though arguments 1 and 2 are integers.  I don't know if systemd
> supports that but if for example a client adds a match with this kind
> of filter, the service must also support that or the signal won't pass
> through the filter.
>

That is the problem with systemd, it does not explicitly document what 
is and what is not supported.  Having glanced at their bloom filter code 
again, I'm pretty sure only string arguments are currently supported and 
non-string arguments in between are not skipped.  This is why our code 
looks the way it does.

Can you also take a look and confirm?  We are stuck supporting only what 
systemd supports.

>>
>>
>>> ---
>>>    ell/dbus-message.c | 38 ++++++++++++++++++++++++++++----------
>>>    1 file changed, 28 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/ell/dbus-message.c b/ell/dbus-message.c
>>> index f223c70..a123a8a 100644
>>> --- a/ell/dbus-message.c
>>> +++ b/ell/dbus-message.c
>>> @@ -1412,9 +1412,12 @@ bool _dbus_kernel_calculate_bloom(struct
>>> l_dbus_message *message,
>>>          const char *signature;
>>>          void *body;
>>>          size_t body_size;
>>> -       struct l_dbus_message_iter iter;
>>> +       struct l_dbus_message_iter iter, tmp;
>>>          uint8_t argn;
>>>          char buf[256];
>>> +       bool (*skip_entry)(struct l_dbus_message_iter *);
>>> +       bool (*get_basic)(struct l_dbus_message_iter *, char, void *);
>>> +       char type;
>>>
>>>          /* The string "interface:" suffixed by the interface name */
>>>          attr = l_dbus_message_get_interface(message);
>>> @@ -1458,17 +1461,32 @@ bool _dbus_kernel_calculate_bloom(struct
>>> l_dbus_message *message,
>>>
>>>          body = _dbus_message_get_body(message, &body_size);
>>>
>>> -       if (_dbus_message_is_gvariant(message))
>>> -               _gvariant_iter_init(&iter, message, signature, NULL,
>>> -                                       body, body_size);
>>> -       else
>>> +       if (_dbus_message_is_gvariant(message)) {
>>> +               if (!_gvariant_iter_init(&iter, message, signature, NULL,
>>> +                                               body, body_size))
>>> +                       return false;
>>> +
>>> +               skip_entry = _gvariant_iter_skip_entry;
>>> +               get_basic = _gvariant_iter_next_entry_basic;
>>> +       } else {
>>>                  _dbus1_iter_init(&iter, message, signature, NULL,
>>> -                               body, body_size);
>>> +                                       body, body_size);
>>> +
>>> +               skip_entry = _dbus1_iter_skip_entry;
>>> +               get_basic = _dbus1_iter_next_entry_basic;
>>> +       }
>>>
>>>          argn = 0;
>>>
>>> -       while (*signature == 's' || *signature == 'o' || *signature ==
>>> 'g') {
>>> -               if (!message_iter_next_entry(&iter, &attr))
>>> +       do {
>>> +               type = l_dbus_message_iter_get_type(&iter);
>>> +               if (!strchr("sog", type))
>>> +                       goto next;
>>> +
>>> +               /* Don't let get_basic move iter->pos without moving
>>> sig_pos */
>>> +               memcpy(&tmp, &iter, sizeof(tmp));
>>> +
>>> +               if (!get_basic(&tmp, type, &attr))
>>>                          return false;
>>
>>
>> Is this to avoid double skipping this entry?  Can we find a less hacky way
>> to do this?  Perhaps peek_basic, or restructure the loop in some way?
>
> The problem is that get_basic moves the data position (iter->pos) but
> doesn't move the iter->sig_pos, as fas as I can see.  Maybe instead we

I'm pretty sure it does.  See bottom of _dbus1_iter_next_entry_basic() 
for example.  However, the signature handling is full of nasty details. 
  See below.

> should just do iter->sig_pos += 1, similarly to what
> message_iter_next_entry_valist does?

It is really bad form to mess with iterator's internal data structures 
outside its core functions.  If you need it to do something specific, 
lets introduce an iterator method specific to your usecase.

Hence my suggestion to restructure the loop or introduce new operations.

Regards,
-Denis

>
> Best regards
>