Re: (original) Diff for flood_net_ssl.c, please take out one added line

[Norman Tuttle <nt...@photon.poly.edu>]

In the diff introduced below which took recursive calls out of the
flood_net_ssl.c and added some conditions to the ssl_read_socket() routine
which emulated the recursive behavior, I had a line which clashed with the
added conditions. If you are adding the diff which makes the changes to
this file, please take out the line:
   if (sslError!=SSL_ERROR_WANT_READ) *buflen = 0;
because it tries to compensate for errors by setting the bytes written to
zero (which may be useful for correct reporting but may not be necessary),
but also will limit the buffer length for future reading to zero (bad
idea) in a case where the error is other than an SSL_ERROR_WANT_READ,
whereas there are now other errors which introduce this continued reading.

-Norman Tuttle, developer, OpenDemand Systems, ntuttle@opendemand.com

On Mon, 13 Oct 2003, Norman Tuttle wrote:

> To Apache Flood development team:
> 
> As part of our work on the Flood code (in our usage of it as par of an
> enterprise product), I have been instructed to contribute back our changes
> on a daily basis so we can both contribute them back to the product and
> get back inportant feedback on these changes.
> 
> Other than some small touch-ups, these changes to flood_net_ssl.c involve
> 1) taking recursive function code out of socket read/write functions,
> (replacing with do .. while loop) resulting in
> a) more robust code, with less possibility of stack-related issues.
> b) errors returned in recursively-called code were not being propagated.
> c) with iterative code it is easier to set limits on the amount of
> iteration, if necessary. I have not changed the logic here (yet).
> 
> and
> 2) the addition of certain cases which should cause a continuation of
> reading.
> 
> The -u3 diff is taken from the current directory holding the modified file
> and the \flood-1.1 directory holding the original from Apache project, and
> is attached here as fns.diff since as inline the mailer would modify the
> text.
> 
> -Norman Tuttle, OpenDemand Systems Developer ntuttle@opendemand.com

Redo for diff: Flood: New patch to flood_socket_keepalive.c fixes keepalive protocol bugs [Ref A7]

[Norman Tuttle <nt...@photon.poly.edu>]

Here is the patch again for flood_socket_keepalive relating to the
protocol, diff done correctly this time. The patches fix actual client
issues we have had with sites, and is explained in what I previously
wrote to the group, quoted below (note that it does not touch issues
related to chunking, which I also mentioned in that post, other than to
replace the assert() with something more user-friendly):

-Norman Tuttle, developer, OpenDemand Systems ntuttle@opendemand.com

On Wed, 22 Oct 2003, Norman Tuttle wrote:

> The new diff available for Flood fixes protocol errors for keepalive (file
> flood_socket_keepalive.c, function keepalive_recv_resp()) mainly by
> assimilating the functionality of the wantresponse and !wantresponse modes
> within the readback operations, and maintaining whatever worked correctly.
> Also the algorithm for reading a decimal value within the above function
> was simplified (we found cases where what was replaced did not work), as
> was the routine for reading a hexadecimal value (this being in the
> function keepalive_read_chunk_size()), the same reason for its replacement
> applying.

In addition, I applied some fixes which would yield a more correct byte
count, and replaced the assert() with something more user-friendly.
Even after these changes, there are some gaping holes in the logic
connected with chunked transfer protocol. Besides not being applied in the
"wantresponse" case, it would clearly be unable to field a chunk size
which was greater than the MAX_DOC_LENGTH.

Flood patch: Fixes hangup bug with SSL and generic sockets [Ref A8]

[Norman Tuttle <nt...@photon.poly.edu>]

Flood developers:

I discovered an error where my system would simply lock up (with maximum
CPU utilization from Flood.exe!) while trying to access an https page,
which is using a server-side certificate, when running Flood from Windows
(did not experience this problem in either Linux. My current SSL package
in use there is openssl-0.9.7b. I did some debugging, and found that there
is a spurious SSL error occurring (SSL_ERROR_SYSCALL). When associated
with a non-zero errno (coming from somewhere in the ssl_open_socket()
routine), this generated an APR_EGENERAL return for ssl_read_socket()
(ERR_print_errors_fp() did not provide any output in this case). Then the
calling code in flood_socket_generic.c apparently does not handle this
error well. I intend to send a follow-up email both to this group and to
the openssl group regarding this issue, but I have a fix available since
the packet is actually coming back successfully (so I am treating this as
a ghost error for now).

The patch I am providing (attached here) to fix this bug and stop the
lock-up affects two files:

flood_net_ssl.c (ssl_read_socket() function): Sets errno=0 before doing
the SSL read so that the check on errno generates an APR_EOF.
flood_socket_generic.c (generic_recv_resp() function): Handle errors more
robustly by not exiting a while loop under any non-APR_SUCCESS status
condition. The function exit, however, will not record an APR_EOF code as
an error since it is the usual way for ending the function.

flood_socket_keepalive.c probably needs similar work which will be posted
as a separate diff in the near future.

-Norman Tuttle, developer, OpenDemand Systems ntuttle@opendemand.com

Flood: New patch to flood_socket_keepalive.c fixes keepalive protocol bugs [Ref A7], Question on Chunking Protocol

[Norman Tuttle <nt...@photon.poly.edu>]

The new diff available for Flood fixes protocol errors for keepalive (file
flood_socket_keepalive.c, function keepalive_recv_resp()) mainly by
assimilating the functionality of the wantresponse and !wantresponse modes
within the readback operations, and maintaining whatever worked correctly.
Also the algorithm for reading a decimal value within the above function
was simplified (we found cases where what was replaced did not work), as
was the routine for reading a hexadecimal value (this being in the
function keepalive_read_chunk_size()), the same reason for its replacement
applying.

Since this diff is independent of other changes made recently, I have done
it off the original file in CVS/Flood 1.1, which I have in my local
directory c:\flood-1.1

Norman Tuttle, developer, OpenDemand Systems, ntuttle@opendemand.com

PS: In a comparison between the 2 modes mentioned above (wantresponse and
!wantresponse), the Chunked Transfer-Encoding method of reading of the
body is absent from the wantresponse pathway. I was wondering whether that
was inadvertent, and if so, it looks like we will have to add this
functionality as well or our readback of data in that case will be
incomplete.

New Flood diff to handle "100 continue" when in first chunk, and next packet available [Ref A5]

[Norman Tuttle <nt...@photon.poly.edu>]

The new patch is the first step towards Flood handling "100 continue" 
responses. If such response is found in the first chunk read, and the 
next response is also available there already, then we read past the 100 
message and read in the next message's headers and body if available.

Please evaluate the attached diff file and incorporate into the 
flood_socket_keepalive.c file in CVS. I have made the diff off the build 
1.1 version and not off my own previous changes since this is an 
independent change.

-Norman Tuttle, developer, OpenDemand Systems, ntuttle@opendemand.com