[PATCH] dbus_new_handlers: Don't send NULL to dbus_message_new_error

[Paul Stewart]

On Wed, Oct 6, 2010 at 7:02 AM, Dan Williams <dcbw at redhat.com> wrote:
> On Tue, 2010-10-05 at 12:07 -0700, Paul Stewart wrote:
>> From: Paul Stewart <pstew at google.com>
>>
>> The new DBus API helper function wpas_dbus_error_unknown_error
>> function can be called as a result of a failure within internal
>> getter calls, which will call this function with a NULL message
>> parameter. ?However, dbus_message_new_error looks very unkindly
>> (i.e, abort()) on a NULL message, so in this case, we should not
>> call it.
>>
>> I've observed this course of events during a call to
>> wpas_dbus_getter_bss_wpa with a faileld parse of the IE parameter.
>> We got here through a call to fill_dict_with_properties which
>> explicitly calls getters with a NULL message parameter. ?Judging
>> from the way it is called, this could easily occur if an AP sends
>> out a malformed (or mis-received) probe response. ?I usually run
>> into this problem while driving through San Francisco, so I'm
>> exposed to any number of base stations along this path.
>
> I'm thinking that instead, we should just pass the DBusMessage down to
> fill_dict_with_properties() so that they can do something intelligent
> with the error. ?That would probably ?mean changing the return type of
> fill_dict_with_properties() to a DBusMessage* which would be NULL on
> success, and an error message on error.
>
> Dan
>

The nature of fill_dict_with_properties is that it iterates over the
properties and calls all getters.  The current behavior is tolerant of
returned errors (continues if they occur) so that if there are any
successfully retrieved properties, they are returned to the callers.
Callers currently return their own error ("No readable properties in
this interface") if no properties are returned.  I don't think
immediately replying to the caller with an error about a single
property element in an iteration makes sense in this case.

Also, there are times, such as the case where I ran into this problem,
when there wasn't actually an incoming message to respond to, and
instead, this was an outgoing signal:

wpas_dbus_getter_bss_wpa <- fill_dict_with_properties  <-
wpa_dbus_get_object_properties <- wpas_dbus_signal_bss

so, I believe that even if we resolved the first issue above, there
are still legitimate cases where the reply_to would be NULL.

--
Paul

>> ---
>> ?wpa_supplicant/dbus/dbus_new_handlers.c | ? 10 ++++++++++
>> ?1 files changed, 10 insertions(+), 0 deletions(-)
>>
>> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
>> index 8e87e20..b9f6a8d 100644
>> --- a/wpa_supplicant/dbus/dbus_new_handlers.c
>> +++ b/wpa_supplicant/dbus/dbus_new_handlers.c
>> @@ -117,6 +117,16 @@ static char * wpas_dbus_new_decompose_object_path(const char *path,
>> ?DBusMessage * wpas_dbus_error_unknown_error(DBusMessage *message,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? const char *arg)
>> ?{
>> + ? ? /*
>> + ? ? ?* This function can be called as a result of a failure
>> + ? ? ?* within internal getter calls, which will call this function
>> + ? ? ?* with a NULL message parameter. ?However, dbus_message_new_error
>> + ? ? ?* looks very unkindly (i.e, abort()) on a NULL message, so
>> + ? ? ?* in this case, we should not call it.
>> + ? ? ?*/
>> + ? ? if (message == NULL)
>> + ? ? ? ? ? ? return NULL;
>> +
>> ? ? ? return dbus_message_new_error(message, WPAS_DBUS_ERROR_UNKNOWN_ERROR,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? arg);
>> ?}
>
>
>