Re: [evolution-patches] ABR patch for camel's rfc2047 encode_string

[Suresh Chandrsekharan <suresh chandrasekharan sun com>]

Ofcourse. But from the bug comments seems like the resposibility for using the bad function came to Sun. But it was actually suggested by Jeff.

In short the patch was made bad by the review. I'm OK wth it, everybody makes mistakes, but should not point to others for ones' oversight.
 

W liście z czw, 29-07-2004, godz. 22:10, Not Zed pisze:

Well Jeff just acknowledged that we need to review patches more closely.  I didn't spot it either, and i should have I guess, I had equal opportunity and responsibility in this regard.

How is that inconsistent with the archives?


On Thu, 2004-07-29 at 22:10 -0700, Suresh Chandrsekharan wrote:

No finger pointing intented, but it was Jeff who told me to use camel_mime_is_lwsp, in the original patch I sent this call was not there.

http://bugzilla.ximian.com/showattachment.cgi?attach_id=7767

This is jeff's mail

http://lists.ximian.com/archives/public/evolution-patches/2004-May/005538.html

Ofcourse it is my mistake to not to check this macro for overflow.

So Jeff, could you pl. check the archives in future before putting comments like the following in the bug report ?

http://bugzilla.ximian.com/show_bug.cgi?id=62029

------ Additional Comments From Jeff Stedfast 2004-07-28 14:14 -------

*groan* guess we gotta check Sun patches more closely. they changed
the code to use camel_mime_is_lwsp() from g_unichar_is_space() - the
is_lwsp() macro uses a 256-byte lookup table and iso8859-2 chars just
so happen to be >256 thus reading memory past the end of the table.

above patch fixes that.


Best,
Suresh

W liście z czw, 29-07-2004, godz. 21:28, Not Zed pisze:

On Thu, 2004-07-29 at 10:20 -0400, Jeffrey Stedfast wrote:

On Thu, 2004-07-29 at 11:01 +0800, Not Zed wrote:
> 
> Maybe we should put it right the macro's?  The compiler should
> optimise it out for a char type.

I don't think we should because it would make it such that we couldn't
do:

 camel_mime_is_foo (*inptr++)

yeah we could, it would just be a trickier macro.  but then again maybe it wouldn't work outside gcc.  i'd be a lot happier, if like the kernel, we just enforced gcc.

we don't currently use that convention, but it'd be nice (especially
since the similar libc versions allow it).

also: since as you pointed out, there's no actual ABR, I don't think we
want to change the macros.

I've checked all the code that uses camel_mime_is_*() macros and checked
that the usage is correct (eg. not using 32bit ints or some such) and
the only case that was wrong was the case fixed in my patch.

> 
> We should also check against ((unsigned int)x) < 256 since -100 < 256
> but not in range for the tables.

gunichar is by definition a uint32 so no need to cast in my patch (I
think you only meant for this cast to be used in the macro's anyway).

yeah.

Since you OK'd my patch, I'm going to commit.

Jeff

> 
> On Thu, 2004-07-29 at 10:16 +0800, Not Zed wrote:
> > 
> > looks ok, should check other instances where we don't pass a char to
> > the type functions.
> > 
> > although this shouldn't abr since we explictly cast to unsigned char
> > which can only be 0-255.
> > 
> > 
> > On Wed, 2004-07-28 at 14:01 -0400, Jeffrey Stedfast wrote:  
> > > likely culprit for http://bugzilla.ximian.com/show_bug.cgi?id=62029
> > > 
> > > when Sun sent us a patch to use is_lwsp() instead of g_unichar_is_space
> > > (), they forgot to make sure that 'c' was <256 (otherwise it would read
> > > past the lookup table and get a bogus value).
> > > 
> > > Jeff
> > > 
> > > Plain text document attachment (62029.patch)
> > > ? 24026.patch
> > > ? 55280.patch
> > > ? 61551.patch
> > > ? 62029.patch
> > > ? ChangeLog.nonximian
> > > ? body
> > > ? body.c
> > > ? body.txt
> > > ? body2.txt
> > > ? braindamaged.patch
> > > ? cf.c
> > > ? charset-map.c
> > > ? charset-map.diff
> > > ? class.sh
> > > ? cmsutil.c
> > > ? date.patch
> > > ? flags
> > > ? foo
> > > ? foo.txt
> > > ? foo2.txt
> > > ? gw-body.txt
> > > ? imap
> > > ? invalid-content-id.patch
> > > ? iso
> > > ? iso.c
> > > ? lang.patch
> > > ? namespace.sh
> > > ? patch
> > > ? pop3-uidl.patch
> > > ? smime
> > > ? uid-cache.patch
> > > ? providers/tmp
> > > ? providers/local/camel-mozilla-folder.c
> > > ? providers/local/camel-mozilla-folder.h
> > > ? providers/local/camel-mozilla-store.c
> > > ? providers/local/camel-mozilla-store.h
> > > ? tests/data/camel-mime-tests.tar.gz
> > > Index: ChangeLog
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/camel/ChangeLog,v
> > > retrieving revision 1.2229
> > > diff -u -r1.2229 ChangeLog
> > > --- ChangeLog	28 Jul 2004 16:44:08 -0000	1.2229
> > > +++ ChangeLog	28 Jul 2004 18:00:08 -0000
> > > @@ -1,3 +1,8 @@
> > > +2004-07-28  Jeffrey Stedfast  <fejj novell com>
> > > +
> > > +	* camel-mime-utils.c (camel_header_encode_string): Fixed an ABR
> > > +	that may have been responsible for bug #62029.
> > > +
> > >  2004-07-26  Jeffrey Stedfast  <fejj novell com>
> > >  
> > >  	* camel-charset-map.c (camel_charset_best_mask): Changed the logic
> > > Index: camel-mime-utils.c
> > > ===================================================================
> > > RCS file: /cvs/gnome/evolution/camel/camel-mime-utils.c,v
> > > retrieving revision 1.210
> > > diff -u -r1.210 camel-mime-utils.c
> > > --- camel-mime-utils.c	18 Jun 2004 20:07:09 -0000	1.210
> > > +++ camel-mime-utils.c	28 Jul 2004 18:00:09 -0000
> > > @@ -1335,7 +1335,7 @@
> > >  			continue;
> > >  		}
> > >  		
> > > -		if (camel_mime_is_lwsp (c) && !last_was_space) {
> > > +		if (c < 256 && camel_mime_is_lwsp (c) && !last_was_space) {
> > >  			/* we've reached the end of a 'word' */
> > >  			if (word && !(last_was_encoded && encoding)) {
> > >  				/* output lwsp between non-encoded words */
> > -- 
> > 
> > Michael Zucchi <notzed ximian com>
> > "born to die, live to work, it's
> > all downhill from here"
> > Novell's Evolution and Free
> > Software Developer
> -- 
> 
> Michael Zucchi <notzed ximian com>
> "born to die, live to work, it's
> all downhill from here"
> Novell's Evolution and Free
> Software Developer

--

Michael Zucchi <notzed ximian com> "born to die, live to work, it's all downhill from here" Novell's Evolution and Free Software Developer

--

Michael Zucchi <notzed ximian com> "born to die, live to work, it's all downhill from here" Novell's Evolution and Free Software Developer