On Wednesday 19 of September 2012 11:20:26 Paul Menzel wrote:
On Wednesday 19 of September 2012 11:20:26 Paul Menzel wrote:
> For whatever reasons, addressbooks can be corrupt. Clicking on such an
> entry in the Contacts overview crashes Evolution with a segmentation
> fault. In the overview the first line of the excerpt is empty.
>
> Therefore the return values have to be checked, which is also good
> programming practice.
<snip>
> ---
> What should be done in `e_destination_set_contact()` in the error case?
> A warning to the user would be nice telling her/him to edit the
> addressbook or Evolution should fix up the addressbook entry itself but
> also notifying the user.
>
> addressbook/libebook/e-destination.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/addressbook/libebook/e-destination.c
> b/addressbook/libebook/e-destination.c index 96582d1..9808fe8 100644
> --- a/addressbook/libebook/e-destination.c
> +++ b/addressbook/libebook/e-destination.c
> @@ -463,8 +463,13 @@ e_destination_set_contact (EDestination *dest,
>
> raw = e_vcard_attribute_get_value (attr->data);
> addr = camel_internet_address_new ();
> - camel_address_unformat (CAMEL_ADDRESS (addr), raw);
> - camel_internet_address_get (addr, 0, &name, &email);
> + if (camel_address_unformat (CAMEL_ADDRESS (addr),
raw) <= 0) {
> + /* Report an error that contact is corrupt */
> + }
> +
> + if (!camel_internet_address_get (addr, 0, &name,
&email)) {
> + /* Report an error corrupt */
> + }
>
> e_destination_set_name (s_dest, name);
> e_destination_set_email (s_dest, email);
In general I agree with your approach, but I don't know whether it makes sense
to report error to user here (can there be other reasons for
camel_internet_address_get() to fail other then addressbook corruption?).
I'd simply suggest something like:
if (camel_address_unformat (CAMEL_ADDRESS (addr), raw) > 0) {
if (camel_internet_address_get (addr, 0, &name, &email)) {
e_destination_set_name (s_dest, name);
e_destination_set_email (s_dest, email);
}
}
I think the _actual_ problem is, that name and email variables are not
initialized before being passed to camel_internet_address_get(). The crash
comes from g_strdup() in e_destination_set_{name,email}() which tries to
duplicate an invalid memory. According to docs, g_strdup(NULL) returns NULL,
so that would prevent any crash in the first place. But as you said above,
checking return value is a good practice, so combining these two would be
valid solution.
[ Off-topic: could we consider enforcing stricter compiler warnings? Getting
"possible use of uninitialized value" and similar warnings would for prevent
us from introducing crashes like this one. ]
Cheers,
Dan
--
dvratil redhat com | Associate Software Engineer / BaseOS / KDE, Qt
GPG Key: 0xC59D614F6F4AE348
Fingerprint: 4EC1 86E3 C54E 0B39 5FDD B5FB C59D 614F 6F4A E348Attachment:
signature.asc
Description: This is a digitally signed message part.