Created attachment 111265 [details] [review]
Fix multi-line vcard value loop

There are a few problems with this code, at e-vcard.c:e_vcard_to_string_vcard_30():904.  First, it calls g_utf8_offset_to_pointer() with an offset of 74 always, even when the string is not 74 characters long anymore: since that function doesn't stop at the end of the string this will lead to memory being accessed beyond the end of the buffer.  We don't write it or do anything with it so on most systems it's probably not going to cause problems, but the C standard does require that you not access memory that has not been allocated.

The second problem is potentially more problematic: we check to see if we're done writing by comparing startptr + len bytes.  If we have UTF8 chars that are larger than 1 byte, then this comparison is WRONG and will cause us to exit the loop too early.

All of this is avoided if we merely loop on the value of len, which counts UTF8 chars rather than bytes, and we exit the loop when we know we don't have enough chars left to overflow the line.

That's what this patch does.

Ross: Can you look at this e-vcard patch ? Thanks.

I do not agree with "the second problem", because this, as it is, is correct, no need to change it (I do not like while(1), btw). It's not a problem, because g_utf8_offset_to_pointer returns pointer moved by 74 UTF characters. And because it is a pointer, then the comparision is correct. Please keep it there.

Otherwise patch looks fine, thanks for reporting and writing it. Just please update the patch and do not forget for ChangeLog entry. Thanks in advance.

Hm; I may have misread the code.  I remember thinking the conditional on the while loop said "while (pos2 < attr_str->str + len)" but looking again I see it actually says "while (pos2 < attr_str->str + attr_str->len)".  Assuming attr_str->len is counting bytes, not UTF chars, then this is OK.

I'll rework the patch.

*** Bug 500509 has been marked as a duplicate of this bug. ***

Any updates on patch ? 
i am getting crash very often in trunk (2.23.3). Please see bug 537585  

(In reply to comment #6)
> i am getting crash very often in trunk (2.23.3). Please see bug 537585  

you can mark your bug as duplicate of this, seems to me

Wow I had no idea this was causing any crashes.  I only found it by perusing valgrind output; it didn't cause my system to crash.  However I agree that these core dumps do seem to be related to this bug.

Anyway, the patch I posted is not wrong as-is, it just needs to be changed to avoid while(1) for Milan.  Although the "second problem" is indeed not a problem, nevertheless I think the patched change is still the cleanest way to fix the "first problem"; certainly it doesn't hurt to conditionalize the loop based on the actual number of characters rather than on the length of the string.

I'll try to bang out a new version of this patch today.

Hrm.  Milan; is it any kind of unconditional loop construct you dislike, or just while (1)?  I mean, is something like "for (;;)" better, or is it the entire idea of having a loop where the exit point is not in the operator line?

I ask because, looking at the code again for a few minutes, I don't see any nice way to write the code without a unconditional loop, that doesn't involve replicating the test (or else replicating some of the code).  Personally I think the replicated test is MUCH worse as it's very confusing to the reader ("didn't we just test that?  Has it changed?  What's going on here?").  Basically we want to stop processing the loop before the last line (the g_utf8_offset_to_pointer) if we don't have > 74 chars.  My while(1) method looks like this:

while (1) {
	g_string_append_len (fold_str, pos1, pos2 - pos1);
	g_string_append (fold_str, CRLF " ");
	pos1 = pos2;
	if (len <= 74)
		break;
	pos2 = g_utf8_offset_to_pointer (pos2, 74);
	len -= 74;
}

The clearest method I can come up with that doesn't use an unconditional loop would look something like this:

do {
	g_string_append_len (fold_str, pos1, pos2 - pos1);
	g_string_append (fold_str, CRLF " ");
	pos1 = pos2;
	if (len > 74) {
		pos2 = g_utf8_offset_to_pointer (pos2, 74);
		len -= 74;
	}
} while (len > 74);


Personally I like the while(1) version better as I think it's easier to read and understand, but YMMV.  Let me know.

OK, I understand. The while(1) is just a personal feeling, nothing important. We clarified the second issue too, so it should be fine as well. And then I'm seeing this, I will take finally the initial patch. I'm so sorry I bothered you with this so long. You can commit to trunk and stable with appropriate ChangeLog entry.

Paul, can you please get the patch in before Monday's release (2.23.4), tia.

I don't have commit privileges, so I can't commit the change.  At least I'm pretty sure I don't; I've never handed over any SSH public keys or anything.

If it helps here's a changelog entry for addressbook/ChangeLog:


2008-06-12  Paul Smith  <psmith@gnu.org>

	** Fix for bug #534111
	Copying past the end of the buffer can cause core dumps.

	* libebook/e-vcard.c (e_vcard_to_string_vcard_30): Loop on the # of
	chars rather than the length of string, and break out before copying
	past the end of the string.

Committed to trunk. Committed revision 8970.
Committed to gnome-2-22. Committed revision 8971.

Thanks Paul, and I'm sorry for the confusion I caused.

No problem; happy it got fixed.  I'm assuming there will be a 2.22.3 or similar coming out at some point as well?  I need to keep on the Ubuntu guys to be taking these fixes into 8.04.  Cheers!

*** Bug 537585 has been marked as a duplicate of this bug. ***