bug in the proxy module ...

[Wesley Craig]

I think this is fundamentally the same as calling select, no?

I've done a little more analysis of when proxy_check_input() is  
called -- just two places in imapd.  The first is when proxying for  
the IDLE command.  In that case, the MUA issues the IDLE command, and  
if the backend supports IDLE, the proxy sends the IDLE command to the  
backend.  The proxy then sits for whatever the idle timeout is in  
proxy_check_input().  The old version of this code was fine, because  
proxy_check_input() is called in a loop.  So if there was more data,  
it would be copied on the next pass.  A slight refinement might be to  
prioritize copying from the server to the client over observing that  
the client has provided new input, thus breaking the loop.

The other imapd call to proxy_check_input() is in the main loop,  
prior to accepting the next command.  Prioritizing copies over  
checking for client input would probably improve this case as well.

The pop3d bug in particular is related to how bitpipe() is written,  
i.e., it doesn't have any protocol knowledge at all.  You'll also  
notice that bitpipe() is responsible for flushing the data,  
proxy_check_input() doesn't do that.  Changing proxy_check_input() to  
handle server to client IO copies first and then return would give  
the outer loop a chance to run.

Simply backing out this change:

	https://bugzilla.andrew.cmu.edu/cgi-bin/cvsweb.cgi/src/cyrus/imap/ 
proxy.c.diff?r1=1.3;r2=1.4

will probably fix pop3d.  A "more correct" fix for the problem  
referenced in that change:

	make sure we send all available data, not just one buffer full.
	this solves a pipelining problem where a response to a command run  
on a proxy
	could be output in the middle of a response to a command run on a  
backend

is probably the re-ordering.  I think there is in fact no need to  
"send all available data", since the outer loop should see to it that  
proxy_check_input() is called multiple times.  Ideally, the outer  
loop would also encapsulate whatever protocol specific knowledge is  
required -- none in the case of callers like bitpipe(), significant  
in the case of imapd.

Perhaps someone knows how to exercise the pipelining problem  
referenced in the above change?

:wes

ps The "reordering" can probably be achieved by adding an else to  
this if:

	if ((err = prot_error(pin)) != NULL) {
		...
	} else {
		return 0;
	}

... after backing out the change above.  This way proxy_check_input()  
will never tell the caller that there's "unhandled" input from the  
client until no IO copying can be done.


On 29 Jul 2008, at 03:54, Poujol Christophe wrote:
> This is a proposal to solve the possible dead lock in proxy.
>
> The problem was : if the length of the  last packet  received by  
> the proxy from one back process is 4096 bytes, the proxy expects to  
> receive data and the back process waits for commands.
>
> I propose you a shortcut :
> if the proxy receives 4096 bytes the proxy puts back the last byte  
> into the buffer and it sends the first 4095 bytes through the  
> outputstream
> (the last byte will be read at the next loop).
> if the proxy receives less than 4096 bytes it works like previously.
>
> The initial code could be modified from  :
>
>  <quote>
>
>  if (pout) {
>                const char *err;
>                char buf[4096];
>                int c;
>
>                do {
>                    c = prot_read(pin, buf, sizeof(buf));
>
>                    if (c == 0 || c < 0) break;
>                    prot_write(pout, buf, c);
>                } while (c == sizeof(buf));
>
>                if ((err = prot_error(pin)) != NULL) {
>  </quote>
>
>
>
> into
>
>  <quote>
>
>   if (pout) {
>                const char *err;
>                char buf[4096];
>                int c;
>
>                do {
>                    c = prot_read(pin, buf, sizeof(buf));
>
>                    if (c == 0 || c < 0) break;
>
>                    if (c == sizeof(buf)) {
>                        prot_ungetc(buf[sizeof(buf) - 1], pin);
>                        prot_write(pout, buf, c - 1);
>                    }
>                    else {
>                        prot_write(pout, buf, c);
>                   }
>
>                } while (c == sizeof(buf));
>
>                if ((err = prot_error(pin)) != NULL) {
>
>  </quote>
>
> -----Message d'origine-----
> De : cyrus-devel-bounces at lists.andrew.cmu.edu [mailto:cyrus-devel- 
> bounces at lists.andrew.cmu.edu] De la part de Wesley Craig
> Envoyé : mercredi 11 juin 2008 21:47
> À : Ken Murchison
> Cc : cyrus-devel at lists.andrew.cmu.edu
> Objet : Re: bug in the proxy module ...
>
> On 10 Jun 2008, at 10:07, Ken Murchison wrote:
>> Any suggestions?  I'm off thinking about other things at the moment.
>
> The comment associated with the change is:
>
>         make sure we send all available data, not just one buffer  
> full.
>         this solves a pipelining problem where a response to a  
> command run
> on a proxy
>         could be output in the middle of a response to a command  
> run on a
> backend
>
> Both versions call prot_select() once.  The old code The new code
> (attempts) to copy input to output until end of input, but since it's
> only called prot_select() once, that's a problem.  There are a couple
> of possibilities, perhaps you're more familiar with prot and it's
> byzantine usage, but here's my analysis:
>
>         1) Instead of looping on the size of the read, we loop until
> prot_read() returns == 0 or < 0.  This assumes that pin isn't set to
> allow blocking.  I don't like this solution, since I'm not terribly
> interested in an exhaustive analysis of every possible pin that
> proxy_check_input() might get.  Maybe you know something I don't, tho.
>
>         2) Introduce prot_select() into the read/write loop.  This  
> will
> allow you to know that there's still input available really without
> blocking.  Of course, if it's a very large block of data, you might
> not see the next block, return control to the calling function, and
> get the same pipelining problem mentioned in the CVS log above.
> Assuming you're not worried about that scenario, it's a good solution
> because it introduces the idea that output from the backend server is
> handled prior to input from the client.
>
>         3) Continuing on the precedence idea above, split the loop  
> handling
> so that backend output is always handled first.  Also, always return
> control to the caller if you ever have backend output.  This way,
> you'll only ever take input from the client if the backend isn't
> sending anything.  I doubt this solves the race mentioned in (2),
> either tho.
>
>         4) Restructure the routines calling proxy_check_input to  
> know the
> structure of the commands being sent and the corresponding
> responses.  This is the surest way to fix the above problem, i.e.,
> don't let the proxy server respond to a command until the response to
> the command sent by the backend is done.  Of course, tho is a huge
> pain, probably involving a ton of additional code.