On Thu, Oct 04, 2001 at 05:12:10PM -0400, MATHIHALLI,MADHUSUDAN (HP-Cupertino,ex1)
wrote:
[ Have a few minutes before class starts... ]
> - if (ssl == NULL) {
> - return -1;
> - }
> -
> [snip]
>
>
> => Why is the checking not required ?.. If it's justified, then the
> corresponding check has to be eliminated from ssl_io_hook_write() also..
Yeah, it should be removed from ssl_io_hook_write as well. But,
I wasn't changing that function. =) Checking for null on something
like this isn't our preferred style - if it is NULL, then something
really bad happened anyway. (ssl can't be null under any normal
circumstances here if you see how the code is called - it just
doesn't add any value...)
> -
> - /* read filter */
> - ret = apr_bucket_read(pbktIn, &data, &len, (apr_read_type_e)eMode);
> - if (!(eMode == AP_MODE_NONBLOCKING && APR_STATUS_IS_EAGAIN(ret))) {
> - /* allow retry */
> - APR_BUCKET_REMOVE(pbktIn);
> - }
> - if (ret == APR_SUCCESS && len == 0 && eMode == AP_MODE_BLOCKING)
> - ret = APR_EOF;
> - if (len == 0) {
> - /* Lazy frickin browsers just reset instead of shutting down.
> */
> - /* also gotta handle timeout of keepalive connections */
> - if (ret == APR_EOF || APR_STATUS_IS_ECONNRESET(ret) ||
> - ret == APR_TIMEUP)
> - {
> - if (APR_BRIGADE_EMPTY(pRec->pbbPendingInput))
> - return APR_EOF;
> - else
> - /* Next time around, the incoming brigade will be empty,
> - * so we'll return EOF then
> - */
> - return APR_SUCCESS;
> - }
> -
> [snip]
>
> Why was this removed ?.. The reason why the (len == 0) checking is being
> done is to take care of browsers like IE, which just closes the connection
> upon termination, or a handshake failure..
No, that should all be handled by other people in other places.
If the socket is closed, that is job of the lower level filters
to take care of. mod_ssl should now only concern itself with
data that was read by the core - this was an assumption about
the socket interface that it has no business knowing about.
At least, that is my operative theory.
Any errors such as these will be handled by CORE_IN and it will
pass back the correct error code. mod_ssl shouldn't do anything
here. Remember, previously, it had to handle all of the socket
code - that is gone now.
> [snip]
> - if (eMode != AP_MODE_NONBLOCKING)
> - ap_log_error(APLOG_MARK,APLOG_ERR,ret,NULL,
> - "Read failed in ssl input filter");
> -
> - if ((eMode == AP_MODE_NONBLOCKING) &&
> - (APR_STATUS_IS_SUCCESS(ret) || APR_STATUS_IS_EAGAIN(ret)))
> - {
> - /* In this case, we have data in the output bucket, or we
> were
> - * non-blocking, so returning nothing is fine.
> - */
> - return APR_SUCCESS;
> - }
> - else {
> - return ret;
> - }
> - }
> [snip]
>
> Again - why is it deleted ?.. This is also *required* when
> AP_MODE_NONBLOCKING is set to true - pl. justify.
No, that's not necessarily true. You were previously reading directly
from the socket bucket - you would have to handle EAGAIN and other
cases, but that's just not a valid assumption here. We shouldn't
be concerning ourselves with what the bucket type is at all.
In fact, BLOCKING or NONBLOCKING just doesn't really matter to
anyone other than the person reading from the raw socket - which
mod_ssl no longer does - only the core can read from the socket.
And, in fact, CORE_IN probably won't use the socket bucket for much
longer.
> [snip]
> - if (bio_is_renegotiating(pRec->pbioRead)) {
> - /* we're doing renegotiation in the access phase */
> - if (len >= *readbytes) {
> - /* trick ap_http_filter into leaving us alone */
> - *readbytes = 0;
> - break; /* SSL_renegotiate will take it from here */
> - }
> - }
> [snip]
>
> How are the renegotiations handled ?.. SSL doesn't want you to process the
> data when it's in the middle of a renegotiation..
This trick doesn't get you anything if you look at the code path.
The renegotiation is handled by the process_connection hook.
The idea is that we are reading everything we can possibly read
into the BIO (up to *readbytes or whatever may already be in
ctx->rawb) and then we'll let OpenSSL determine what to do with
it. We'll extract the unencrypted information from OpenSSL via
SSL_read.
If it returns READ_MORE, we could retry, but I don't think we
need to do so. The higher level filters (i.e. request filters)
need to handle that retry.
> [snip]
> + /* Readbytes == 0 implies we only want a LF line.
> + * 1024 seems like a good number for now. */
> + *tempread = 1024;
> [snip]
>
> WHAT ?.. SSL is pretty different from the way HTTP works.. During a SSL
> handshake, there's a tightly coupled message exchange that goes on b/w
> client and server.. client sends something, server interprets and sends back
> something, client interprets and communicates back and so on..
> The actual size of the data communicated in each phase depends upon the
> cert. size, the ciphers available and various other parameters.. You can
> never assume that x bytes will be communicated during a phase phase.. (1024
> - is a assumption according to me)..
No, all we are asking for is 1024 bytes from the lower level (in
this case CORE_IN) when asking for a LF chunk. As you have
mentioned, there is no direct correlation between sockets that were
passed in and what will be sent to ctx->b. All we are saying is
that we are willing to buffer at *MOST* 1024 bytes of data in
mod_ssl's buffer. We'll only return *one* brigade that corresponds
to a LF line pair - why this is in a loop that repeats - we'll be
asking for 1024 bytes at a time from the socket to be passed to
churn_input(). If we get 1024 raw bytes, that may actually be zero
real bytes that are decrypted. That case should be acceptable.
And, if we read 1024 from the core, find the LF in position 50,
we'll set the rest of the bytes to be read at a later time and
return the first 50 bytes of ctx->b.
> So, if SSL asks for more data (SSL_ERROR_WANT_READ), it means that it wants
> *all* the data that's available in the buffer.. If it get buffer less than
> the size it is expecting, then it just cries for more data.. We *have* to
> handle that condition - understand *how much* more data is it asking for..
> The information is *already* available in the SSL context - but I'm afraid
> to use them directly.. There should be some API that should tell *how much*
> data is SSL expecting from the communication channel.
No, I don't think we actually do. I think it would be reasonable to
request data and since SSL eats everything to return 0 bytes (we may
need to insert an immortal bucket of length 0 here). Yes, we could
continually retry, but that seems like a problem for the higher
level to handle. There should be no problem, IMHO, of returning
a zero length brigade to the higher level.
Remember - we are only asking for *AT MOST* readbytes. There is
no contract (this is what Greg and I have been pointing out) that
you will receive exactly readbytes worth of data. In fact, with
mod_ssl, I'll bet you will often get back less. But, it definitely
doesn't make sense to try to increase readbytes in mod_ssl to
handle it. In most cases, I'm going to think that the data will
be an almost 1<->1 comparison (one encyrpted byte may be worth
one unencrypted byte). So, I don't think it would any value to
try to satisfy the complete readbytes. We should and often
will send back less than what they asked for.
Late for class... -- justin
