re-alpine-bugs

[Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Mark C. <mrc...@pa...>]

Here's my present for the project.  I already fixed this in Panda IMAP. 
Rather than submit a patch, I'll tell you what the problem is and let you 
figure it out.  It shouldn't be difficult.  If you like, I'll review your 
patch after you do it and tell you if it's right.

The problems are in imap/src/c-client/imap4r1.c and are caused by 
references to null pointers.

[1] The code block starting at line 3722:
     else if ((!strcmp (s,"FETCH") || !strcmp (s,"STORE")) &&
 	     msgno && (msgno <= stream->nmsgs)) {
assumes that t has a pointer to the response values.  However, t can be
null at this point.

Just include a condition for t being non-null in this if test.

[2] The code at line 3733:
       while (prop = (strtok_r (t," )",&r))) {
 	t = strtok_r (NIL,"\n",&r);
parses a property into prop, and the start of the property value into t. 
If the string terminated with the property, then the property value is 
null, and the subsequent code does not expect it.

You need to test for t being set null here.  If it is null, output an 
error message such as "Missing data for property", set stream->unhealthy, 
and do not continue parsing this property value.  There are numerous 
example of how to output an error message in that routine.  The important 
thing is that you do NOT run the code starting at the INIT_GETS() at line 
3735, and instead resume at the end of that code block at line 3881 
(comment "note envelope if we got one").

You can do this with an if test and a continue.  Somewhat more elegant is 
to move the t assignment within the while, and have the error message come 
out after the while under an "if (prop)" test (since the condition will 
happen only if prop is non-null and t is null).

Well, I kind of told you the patches after all.  Good luck!

-- Mark --

http://panda.com/mrc
Democracy is two wolves and a sheep deciding what to eat for lunch.
Liberty is a well-armed sheep contesting the vote.

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Andraž 'r. L. <ru...@co...>]

:2009-06-09T08:25:Mark Crispin:

> Here's my present for the project.  I already fixed this in Panda IMAP. 
> Rather than submit a patch, I'll tell you what the problem is and let you 
> figure it out.  It shouldn't be difficult.  If you like, I'll review your 
> patch after you do it and tell you if it's right.
> 
> The problems are in imap/src/c-client/imap4r1.c and are caused by 
> references to null pointers.
> 
> [1] The code block starting at line 3722:
>      else if ((!strcmp (s,"FETCH") || !strcmp (s,"STORE")) &&
>  	     msgno && (msgno <= stream->nmsgs)) {
> assumes that t has a pointer to the response values.  However, t can be
> null at this point.
> 
> Just include a condition for t being non-null in this if test.
> 

diff --git a/imap/src/c-client/imap4r1.c b/imap/src/c-client/imap4r1.c
index b938984..d21070c 100644
--- a/imap/src/c-client/imap4r1.c
+++ b/imap/src/c-client/imap4r1.c
@@ -3720,7 +3720,7 @@ void imap_parse_unsolicited (MAILSTREAM *stream,IMAPPARSEDREPLY *r
     }
 
     else if ((!strcmp (s,"FETCH") || !strcmp (s,"STORE")) &&
-            msgno && (msgno <= stream->nmsgs)) {
+            msgno && (msgno <= stream->nmsgs)) && ((t != NULL)){
       char *prop;
       GETS_DATA md;
       ENVELOPE **e;

If I've read point 1 correctly this looks like the code that you said
should go there?


Still trying to figure out point 2. I only had like 3 months of C
in-between a pile of Java.

-- 
Andraž ruskie Levstik
Source Mage GNU/Linux Games/Xorg grimoire guru
Re-Alpine Coordinator http://sourceforge.net/projects/re-alpine/
Geek/Hacker/Tinker

Quis custodiet ipsos custodies.

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Mark C.]

On Tue, 9 Jun 2009, Andraž 'ruskie' Levstik wrote:
>     else if ((!strcmp (s,"FETCH") || !strcmp (s,"STORE")) &&
> -            msgno && (msgno <= stream->nmsgs)) {
> +            msgno && (msgno <= stream->nmsgs)) && ((t != NULL)){

A minor syntax error.  You should have just one opening parenthesis before 
"t":

> +            msgno && (msgno <= stream->nmsgs)) && (t != NULL)){


You can also use
 	t
as equivalent to
 	(t != NULL)
e.g,.

> +            msgno && (msgno <= stream->nmsgs)) && t){

-- Mark --

http://panda.com/mrc
Democracy is two wolves and a sheep deciding what to eat for lunch.
Liberty is a well-armed sheep contesting the vote.

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Eduardo C.]

On Tue, 9 Jun 2009, Mark Crispin wrote:

:) Here's my present for the project.  I already fixed this in Panda IMAP. 
:) Rather than submit a patch, I'll tell you what the problem is and let you 
:) figure it out.  It shouldn't be difficult.  If you like, I'll review your 
:) patch after you do it and tell you if it's right.

Attached is a patch that I believe satisfies what Mark reported. Thank you 
for your review.

-- 
Eduardo
http://staff.washington.edu/chappa/alpine/

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Mark C.]

You got the first part right, but not the second.

If you are going to use the if method, you need to have the scope of your 
if/else cover all the way down to line 3881.  Put another way, you do NOT 
want to execute any of the if/else chain starting with the comment 
"canonicalize property, parse it".

Hence my comment about using a continue statement.  You can put that in 
your error code and not need to have the normal path be under an if/else. 
If you don't want to use a continue, then you must scope nearly 150 lines 
under if/else.

Hence my other comment about the more elegant solution of putting the 
assignment under a while.

Also note that mm_notify (which passes a stream) is usually used here 
instead of mm_log().  It's not a big deal (the code would work either 
way), but Alpine may prefer to know what stream is having problems rather 
than get an unattributed log.

-- Mark --

http://panda.com/mrc
Democracy is two wolves and a sheep deciding what to eat for lunch.
Liberty is a well-armed sheep contesting the vote.

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Mark C.]

I forgot to mention.

Before anyone starts making any patches to the IMAP stuff in the 
development tree (what will it get called?  2.01?  2.10?  dev?), you 
should first replace it with the imap-2007e on UW's FTP server.

There are a couple of important bugfixes there that aren't in the Alpine 
2.00 distribution (which is based upon UW imap-2007c).

-- Mark --

http://panda.com/mrc
Democracy is two wolves and a sheep deciding what to eat for lunch.
Liberty is a well-armed sheep contesting the vote.

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Eduardo C. <ch...@wa...>]

On Tue, 9 Jun 2009, Eduardo Chappa wrote:

:) Attached is a patch that I believe satisfies what Mark reported. Thank 
:) you for your review.

I misread a part of Mark's message. Attached is a patch that follows 
Mark's directions better.

It would be better to have access to such defective server, though.

-- 
Eduardo
http://staff.washington.edu/chappa/alpine/

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Eduardo C. <ch...@wa...>]

On Tue, 9 Jun 2009, Eduardo Chappa wrote:
:) 
:) I misread a part of Mark's message. Attached is a patch that follows 
:) Mark's directions better.

And the patch did not get attached. Aarrg. Sorry for the noise.

-- 
Eduardo
http://staff.washington.edu/chappa/alpine/

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Eduardo C.]

On Tue, 9 Jun 2009, Mark Crispin wrote:

:) You got the first part right, but not the second.

Thank you for your comments Mark. Attached is a revised version. I decided 
to use the "continue;" method, instead of the big  if/else. This should do 
it.

Thank you.

-- 
Eduardo
http://staff.washington.edu/chappa/alpine/

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Mark C.]

On Tue, 9 Jun 2009, Eduardo Chappa wrote:
> Thank you for your comments Mark. Attached is a revised version. I decided
> to use the "continue;" method, instead of the big  if/else. This should do
> it.

I agree.

It's not how I did the fix.  However, the point of this exercise is NOT to 
depend upon me for patches, but rather to get the re-alpine dev team up to 
speed on building their own correct patches.

In any case, the revised patches look like they will work and there is 
nothing obviously wrong with them.  You now need to regression test the 
patches.  It should be quite obvious if you broke something, just by 
opening an IMAP mailbox and viewing the index.

I do not have access to the broken IMAP server; it's O2's server in 
Germany.  However, I know where it cracked (later on when it tried to 
parse the property value), and from that I inferred the cause.  I know 
that my fix in Panda IMAP worked.  Since your patch is functionally 
equivalent to my fix, there is no reason to believe that it won't work.

Good luck!

-- Mark --

http://panda.com/mrc
Democracy is two wolves and a sheep deciding what to eat for lunch.
Liberty is a well-armed sheep contesting the vote.

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Andraž 'r. L. <ru...@co...>]

Thank you for the patch, committed and pushed.

I also merged in the latest imap code so that's done as well.

Regards
-- 
Andraž ruskie Levstik
Source Mage GNU/Linux Games/Xorg grimoire guru
Re-Alpine Coordinator http://sourceforge.net/projects/re-alpine/
Geek/Hacker/Tinker

Ryle hira.

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Andraž 'r. L. <ru...@co...>]

:2009-06-09T10:54:Mark Crispin:

> I forgot to mention.
> 
> Before anyone starts making any patches to the IMAP stuff in the 
> development tree (what will it get called?  2.01?  2.10?  dev?), you 
> should first replace it with the imap-2007e on UW's FTP server.
> 
> There are a couple of important bugfixes there that aren't in the Alpine 
> 2.00 distribution (which is based upon UW imap-2007c).
> 

I take it this is mostly just a replace the current imap with the new
one and forget about it type of operation.

As for the next version I guess 2.10 and I'm thinking release start of
August this year.

If nothing else to get attention of people.

-- 
Andraž ruskie Levstik
Source Mage GNU/Linux Games/Xorg grimoire guru
Re-Alpine Coordinator http://sourceforge.net/projects/re-alpine/
Geek/Hacker/Tinker

Be sure brain is in gear before engaging mouth.

Re: [Re-alpine-bugs] Alpine crashes talking to non-compliant IMAP server

[Mark C. <mrc...@pa...>]

On Wed, 10 Jun 2009, Andraž 'ruskie' Levstik wrote:
> I take it this is mostly just a replace the current imap with the new
> one and forget about it type of operation.

Correct.

> As for the next version I guess 2.10 and I'm thinking release start of
> August this year.

Sounds good!

-- Mark --

http://panda.com/mrc
Democracy is two wolves and a sheep deciding what to eat for lunch.
Liberty is a well-armed sheep contesting the vote.