[systemd-devel] [PATCH 0/1] bus: make sure we always return valid error messages

[Lennart Poettering]

On Sat, 30.11.13 10:45, Djalal Harouni (tixxdz at opendz.org) wrote:

> Hi list,
> 
> The sd_bus_error_copy() is used by bus clients to copy sd_bus_error
> containt. If the 'need_free' field of the sd_bus_error struct of the
> source is false, then strdup() will be avoided and the 'name' and
> 'message' pointer fields of the destination are set to the sources's
> fields. This will be true for all bus clients.
> 
> An example: the libsystemd-bus/sd-bus.c:sd_bus_call() function is using
> an sd_bus_message *incoming heap pointer inside the for() loop.
> 
> If bus_read_message() succeed it will set the 'incoming' sd_bus_message
> and if we have a match and the header type is an
> SD_BUS_MESSAGE_METHOD_ERROR then sd_bus_error_copy() will get called to
> copy the 'incoming->error' into the 'sd_bus_error *error' passed as an
> argument to the sd_bus_call() function. Since 'incoming->error.need_free'
> is false then destination's fields will be just set to source's fields.
> 
> Memory pointed by source's fields may change at any time! There is no
> guarentee that this memory will be available to be used or processed
> later by the sd_bus_error destination. Even the 'incoming' memory will
> be freed right after the sd_bus_error_copy() by sd_bus_message_unref()
> and at this point, the 'error' data might be invalid
> 
> I've experienced truncated error messsages, but not always! it is
> related to lower layers and how bus messages are processed...
> 
> Note: sd_bus_error_copy() behavior was to always strdup() source, it was
> changed in part by commit 780896a4f1ec
> 
> 
> Fix:
> 1) make sd_bus_error_copy() always strdup content of source regardless
> of the 'need_free' flag since it is never set. This will restore
> behavior before commit 780896a4f1ec. This is the easy one.
> 
> The following patch just use this one.
> 
> 
> 2) Add a new sd_bus_error_strdup() API to force strdup() and make
> sd_bus_error_copy() use it ?
> 
> 
> I'm open to propostions, I can write another patch. Thanks!

Hmm, what we could do instead is store more information in
sd_bus_error's need_free field:

< 0 → temporarily const, deep copies necessary to keep around, no free() on free
  0 → forever const, shallow copy OK, no free() on free
> 0 → dynamic, deep copy always, free() on free

I'll make the necessary changes!

Thanks for the pointer!

Lennart

-- 
Lennart Poettering, Red Hat