Re: [Evolution-hackers] =?utf-8?b?W1BBVENIXSBCdWcgIzY4NDE3NSDigJMg?= =?utf-8?q?_Check_for_corrupt_addressbook?=



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



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]