Am Mittwoch, den 19.09.2012, 12:29 +0200 schrieb Dan Vratil:
> On Wednesday 19 of September 2012 11:20:26 Paul Menzel wrote:
> On Wednesday 19 of September 2012 11:20:26 Paul Menzel wrote:
^^
Is putting the line introducing the quote twice a bug in a KMail
version?
> > 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?).
Unfortunately I do not know.
> 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);
> }
> }
Still the user should be warned somehow and told how to fix this.
> 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.
Interesting. I will try to take a look.
> [ 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. ]
Good idea.
Thanks,
Paul
Attachment:
signature.asc
Description: This is a digitally signed message part