RE: finish connection hook (WAS: RE: [PATCH] SSL not sending close alert message)
--On Wednesday, February 25, 2004 1:20 PM -0800 Mathihalli, Madhusudan [EMAIL PROTECTED] wrote: Hooking the cleanups to the connection pool may be a little too late for some modules (Example SSL shutdown/SSL Alert). For filters like SSL, the EOC logic would probably be just fine - I can't think of other filters which cannot use the EOC logic. Correct. A cleanup is too late for this particular action. I'd like to understand what's the inhibition/limitation for adding a new hook - IOW, why are buckets like EOC preferable over a finish_connection hook ? I can guess it's something to do with the performance - but a detailed explaination/pointer to something would be helpful. It's that EOC is the proper parallel to the EOS bucket. We just never realized we needed it. ;-) But, I'm fairly sure that it is the 'right' solution here. Also, I'm not sure that we can add a new hook to 2.0; so if we added a hook to solve this problem, it'd have to wait for 2.2 to be resolved. (We might be able to add it to 2.0, but I'm not 100% sure.) -- justin
Re: [PATCH] SSL not sending close alert message
On Wed, 2004-02-25 at 00:15, Cliff Woolley wrote: On Tue, 24 Feb 2004, Joe Orton wrote: I wasn't sure whether or not this EOC bucket type should go in APR-util or httpd. Filtering gurus, what say ye? That bit looks OK to me otherwise with a licence header added to the new file. I say httpd. +1. There is probably still some cleanup left to do in apr/apr-util of things that are only applicable to httpd. See the comments in the xml code, which are focused a lot on the mod_dav code for example. Not something to worry about too much at this point though, other than keep new httpd specific code in httpd. Sander
Re: finish connection hook (WAS: RE: [PATCH] SSL not sending close alert message)
--On Tuesday, February 24, 2004 4:49 PM -0800 Mathihalli, Madhusudan [EMAIL PROTECTED] wrote: ... brings up the question : do you still want the finish_connection hook OR should the modules be content with the EOC bucket ? I don't think we'd need a new hook. -- justin
Re: finish connection hook (WAS: RE: [PATCH] SSL not sending close alert message)
On Wed, Feb 25, 2004 at 09:16:04AM -0800, Justin Erenkrantz wrote: --On Tuesday, February 24, 2004 4:49 PM -0800 Mathihalli, Madhusudan [EMAIL PROTECTED] wrote: ... brings up the question : do you still want the finish_connection hook OR should the modules be content with the EOC bucket ? I don't think we'd need a new hook. -- justin Rather than a finish_connection hook, I'd just suggest that people attach cleanups to the connection pool. Cheers, -g -- Greg Stein, http://www.lyra.org/
RE: finish connection hook (WAS: RE: [PATCH] SSL not sending close alert message)
-Original Message- From: Greg Stein [mailto:[EMAIL PROTECTED] [SNIP] On Wed, Feb 25, 2004 at 09:16:04AM -0800, Justin Erenkrantz wrote: --On Tuesday, February 24, 2004 4:49 PM -0800 Mathihalli, Madhusudan wrote: ... brings up the question : do you still want the finish_connection hook OR should the modules be content with the EOC bucket ? I don't think we'd need a new hook. -- justin Rather than a finish_connection hook, I'd just suggest that people attach cleanups to the connection pool. Hooking the cleanups to the connection pool may be a little too late for some modules (Example SSL shutdown/SSL Alert). For filters like SSL, the EOC logic would probably be just fine - I can't think of other filters which cannot use the EOC logic. I thought having a finish_connection hook is logical and more intutive - especially since we have the pre_connection/process_connection hooks already. But my sense of logic may not be Apache's logic :) I'd like to understand what's the inhibition/limitation for adding a new hook - IOW, why are buckets like EOC preferable over a finish_connection hook ? I can guess it's something to do with the performance - but a detailed explaination/pointer to something would be helpful. Thanks, -Madhu
RE: [PATCH] SSL not sending close alert message
-Original Message-
From: Joe Orton [mailto:[EMAIL PROTECTED]
[SNIP]
This is just back to what we had patches for already: doing an SSL
shutdown on any EOF bucket, right? Which is not right since you get an
EOS after each HTTP response, not at the end of the connection.
Hence the need for a new bucket type to represent end-of-connection
differently from EOS.
(the test case for that is to see if you can send two requests on a
single SSL connection)
Okay - since I've not played much with buckets, here's my first attempt at getting
something to work - please let me know if it's okay.
Logic:
- When ap_flush_conn() is called (always before apr_socket_close()), create a EOC
bucket and pass it on
- In the SSL (output) filter, invoke SSL_shutdown but don't free the SSL context as
it's required for the flush that happens later
- ssl_filter_io_shutdown() takes a extra flag for freeing the SSL context (when a EOC
is received, it just sends the SSL_shutdown but doesn't do the free)
Patch:
---
NEW FILE: apr-util/buckets/apr_buckets_eoc.c
---
#include apr_buckets.h
static apr_status_t eoc_bucket_read(apr_bucket *b, const char **str,
apr_size_t *len, apr_read_type_e block)
{
*str = NULL;
*len = 0;
return APR_SUCCESS;
}
APU_DECLARE(apr_bucket *) apr_bucket_eoc_make(apr_bucket *b)
{
b-length = 0;
b-start = 0;
b-data= NULL;
b-type= apr_bucket_type_eoc;
return b;
}
APU_DECLARE(apr_bucket *) apr_bucket_eoc_create(apr_bucket_alloc_t *list)
{
apr_bucket *b = apr_bucket_alloc(sizeof(*b), list);
APR_BUCKET_INIT(b);
b-free = apr_bucket_free;
b-list = list;
return apr_bucket_eoc_make(b);
}
APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_eoc = {
EOC, 5, APR_BUCKET_METADATA,
apr_bucket_destroy_noop,
eoc_bucket_read,
apr_bucket_setaside_noop,
apr_bucket_split_notimpl,
apr_bucket_simple_copy
};
---
END of apr_buckets_eoc.c
---
Index: apr-util/include/apr_buckets.h
===
RCS file: /home/cvspublic/apr-util/include/apr_buckets.h,v
retrieving revision 1.156
diff -u -r1.156 apr_buckets.h
--- apr_buckets.h 13 Feb 2004 09:55:26 - 1.156
+++ apr_buckets.h 24 Feb 2004 17:46:31 -
@@ -453,6 +453,12 @@
*/
#define APR_BUCKET_IS_FLUSH(e) ((e)-type == apr_bucket_type_flush)
/**
+ * Determine if a bucket is an End Of Connection (EOC) bucket
+ * @param e The bucket to inspect
+ * @return true or false
+ */
+#define APR_BUCKET_IS_EOC(e) ((e)-type == apr_bucket_type_eoc)
+/**
* Determine if a bucket is an EOS bucket
* @param e The bucket to inspect
* @return true or false
@@ -1056,6 +1062,11 @@
*/
APU_DECLARE_DATA extern const apr_bucket_type_t apr_bucket_type_flush;
/**
+ * The End Of Connection (EOC) bucket type. This signifies that the
+ * connection will be closed.
+ */
+APU_DECLARE_DATA extern const apr_bucket_type_t apr_bucket_type_eoc;
+/**
* The EOS bucket type. This signifies that there will be no more data, ever.
* All filters MUST send all data to the next filter when they receive a
* bucket of this type
@@ -1202,6 +1213,21 @@
* other code should call apr_bucket_foo_create. All the initialization
* functions change nothing if they fail.
*/
+
+/**
+ * Create an End of Connection (EOC) bucket. This indicates that the
+ * connection will be closed.
+ * @param list The freelist from which this bucket should be allocated
+ * @return The new bucket, or NULL if allocation failed
+ */
+APU_DECLARE(apr_bucket *) apr_bucket_eoc_create(apr_bucket_alloc_t *list);
+
+/**
+ * Make the bucket passed in an End Of Connection (EOC) bucket.
+ * @param b The bucket to make into an EOC bucket
+ * @return The new bucket, or NULL if allocation failed
+ */
+APU_DECLARE(apr_bucket *) apr_bucket_eoc_make(apr_bucket *b);
/**
* Create an End of Stream bucket. This indicates that there is no more data
Index: httpd-2.0/server/connection.c
===
RCS file: /home/cvspublic/httpd-2.0/server/connection.c,v
retrieving revision 1.111
diff -u -r1.111 connection.c
--- server/connection.c 1 Jan 2004 13:26:23 - 1.111
+++ server/connection.c 24 Feb 2004 17:24:27 -
@@ -108,10 +108,23 @@
#define MAX_SECS_TO_LINGER 30
#endif
+AP_CORE_DECLARE(void) ap_end_connection(conn_rec *c)
+{
+apr_bucket_brigade *bb;
+apr_bucket *b;
+
+bb = apr_brigade_create(c-pool, c-bucket_alloc);
+b = apr_bucket_eoc_create(c-bucket_alloc);
+APR_BRIGADE_INSERT_TAIL(bb, b);
+ap_pass_brigade(c-output_filters,
Re: [PATCH] SSL not sending close alert message
On Tue, Feb 24, 2004 at 09:59:00AM -0800, Mathihalli, Madhusudan wrote: -Original Message- From: Joe Orton [mailto:[EMAIL PROTECTED] [SNIP] This is just back to what we had patches for already: doing an SSL shutdown on any EOF bucket, right? Which is not right since you get an EOS after each HTTP response, not at the end of the connection. Hence the need for a new bucket type to represent end-of-connection differently from EOS. (the test case for that is to see if you can send two requests on a single SSL connection) Okay - since I've not played much with buckets, here's my first attempt at getting something to work - please let me know if it's okay. I wasn't sure whether or not this EOC bucket type should go in APR-util or httpd. Filtering gurus, what say ye? That bit looks OK to me otherwise with a licence header added to the new file. mod_ssl-side I'd just use the changes in the most recent patch I posted with the CLOSE bucket test replaced with the EOC bucket test in the output filter: in my testing you needed to turn off buffering in bio_filter_out_write to get the shutdown logic working correctly. Thanks Madhu! joe
Re: [PATCH] SSL not sending close alert message
On Tue, 24 Feb 2004, Joe Orton wrote: I wasn't sure whether or not this EOC bucket type should go in APR-util or httpd. Filtering gurus, what say ye? That bit looks OK to me otherwise with a licence header added to the new file. I say httpd. --Cliff
RE: [PATCH] SSL not sending close alert message
-Original Message- From: Joe Orton [mailto:[EMAIL PROTECTED] [SNIP] I wasn't sure whether or not this EOC bucket type should go in APR-util or httpd. Filtering gurus, what say ye? That bit looks OK to me otherwise with a licence header added to the new file. Since the EOC bucket is generic (and not limited to SSL type filters), I thought the best place is in apr-util along with other bucket types ! mod_ssl-side I'd just use the changes in the most recent patch I posted with the CLOSE bucket test replaced with the EOC bucket test in the output filter: in my testing you needed to turn off buffering in bio_filter_out_write to get the shutdown logic working correctly. Well.. I'll try that out - but it appeared that if I release the SSL buffer upon getting a EOC bucket - the shutdown data (alert message) doesn't get flushed. That was the reason I'd to use the shutdown_flag logic. -Madhu
Re: [PATCH] SSL not sending close alert message
On Tue, Feb 24, 2004 at 06:15:20PM -0500, Cliff Woolley wrote: On Tue, 24 Feb 2004, Joe Orton wrote: I wasn't sure whether or not this EOC bucket type should go in APR-util or httpd. Filtering gurus, what say ye? That bit looks OK to me otherwise with a licence header added to the new file. I say httpd. Agreed. -- Greg Stein, http://www.lyra.org/
RE: [PATCH] SSL not sending close alert message
Argh.. stupid e-mail client.. -Original Message- From: Joe Orton [mailto:[EMAIL PROTECTED] [SNIP] I wasn't sure whether or not this EOC bucket type should go in APR-util or httpd. Filtering gurus, what say ye? That bit looks OK to me otherwise with a licence header added to the new file. Since the EOC bucket is generic (and not limited to SSL type filters), I thought the best place is in apr-util along with other bucket types ! mod_ssl-side I'd just use the changes in the most recent patch I posted with the CLOSE bucket test replaced with the EOC bucket test in the output filter: in my testing you needed to turn off buffering in bio_filter_out_write to get the shutdown logic working correctly. Well.. I'll try that out - but it appeared that if I release the SSL buffer upon getting a EOC bucket - the shutdown data (alert message) doesn't get flushed. That was the reason I'd to use the shutdown_flag logic. If the SSL context is released, the flush fails. Buffering is good - especially if we have huge amount data to be processed. I thought it's better not to disturb it. -Madhu
RE: [PATCH] SSL not sending close alert message
-Original Message- From: Cliff Woolley [mailto:[EMAIL PROTECTED] [SNIP] On Tue, 24 Feb 2004, Joe Orton wrote: I wasn't sure whether or not this EOC bucket type should go in APR-util or httpd. Filtering gurus, what say ye? That bit looks OK to me otherwise with a licence header added to the new file. I say httpd. server/eoc_bucket.c ? -Madhu
Re: [PATCH] SSL not sending close alert message
Cliff Woolley wrote: On Tue, 24 Feb 2004, Joe Orton wrote: I wasn't sure whether or not this EOC bucket type should go in APR-util or httpd. Filtering gurus, what say ye? That bit looks OK to me otherwise with a licence header added to the new file. I say httpd. +1 -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
finish connection hook (WAS: RE: [PATCH] SSL not sending close alert message)
-Original Message- From: Joe Orton [mailto:[EMAIL PROTECTED] [SNIP] mod_ssl-side I'd just use the changes in the most recent patch I posted with the CLOSE bucket test replaced with the EOC bucket test in the output filter: in my testing you needed to turn off buffering in bio_filter_out_write to get the shutdown logic working correctly. ... brings up the question : do you still want the finish_connection hook OR should the modules be content with the EOC bucket ? -Madhu
RE: [PATCH] SSL not sending close alert message
Oops.. A typo (in the second block - line 951,6...) during the cut-and-paste
operation. This mail has the corrected version.
-Madhu
-Original Message-
From: Mathihalli, Madhusudan
Sent: Monday, February 23, 2004 1:22 PM
To: '[EMAIL PROTECTED]'
Subject: [PATCH] SSL not sending close alert message
Hi,
I started working on Justin's idea of creating a EOC
bucket - to do a SSL shutdown before the socket close(). But
since the ap_flush_conn is called just before closing the
socket - I thought of doing the SSL shutdown during the flush
itself. Let me know what you think of this patch.
-Madhu
Index: ssl_engine_io.c
===
RCS file: /home/cvspublic/httpd-2.0/modules/ssl/ssl_engine_io.c,v
retrieving revision 1.117
diff -u -r1.117 ssl_engine_io.c
--- ssl_engine_io.c 9 Feb 2004 20:29:22 - 1.117
+++ ssl_engine_io.c 23 Feb 2004 21:18:24 -
@@ -872,7 +872,8 @@
*/
static apr_status_t ssl_filter_io_shutdown(ssl_filter_ctx_t
*filter_ctx,
conn_rec *c,
- int abortive)
+ int abortive,
+ int shutdown_flag)
{
SSL *ssl = filter_ctx-pssl;
const char *type = ;
@@ -951,6 +952,9 @@
SSL_set_shutdown(ssl, shutdown_type);
SSL_smart_shutdown(ssl);
+if (!shutdown_flag)
+return APR_SUCCESS;
+
/* and finally log the fact that we've closed the connection */
if (c-base_server-loglevel = APLOG_INFO) {
ap_log_error(APLOG_MARK, APLOG_INFO, 0, c-base_server,
@@ -990,7 +994,7 @@
}
c = (conn_rec *)SSL_get_app_data(filter_ctx-pssl);
-if ((ret = ssl_filter_io_shutdown(filter_ctx, c, 0)) !=
APR_SUCCESS) {
+if ((ret = ssl_filter_io_shutdown(filter_ctx, c, 0, 1)) !=
APR_SUCCESS) {
ap_log_error(APLOG_MARK, APLOG_INFO, ret, NULL,
SSL filter error shutting down I/O);
}
@@ -1025,7 +1029,7 @@
c-base_server,
SSL Proxy connect failed);
ssl_log_ssl_error(APLOG_MARK, APLOG_INFO, c-base_server);
-return ssl_filter_io_shutdown(filter_ctx, c, 1);
+return ssl_filter_io_shutdown(filter_ctx, c, 1, 1);
}
return APR_SUCCESS;
@@ -1089,7 +1093,7 @@
inctx-rc = APR_EGENERAL;
}
-return ssl_filter_io_shutdown(filter_ctx, c, 1);
+return ssl_filter_io_shutdown(filter_ctx, c, 1, 1);
}
/*
@@ -1130,7 +1134,7 @@
error ? error : unknown);
ssl_log_ssl_error(APLOG_MARK, APLOG_INFO, c-base_server);
-return ssl_filter_io_shutdown(filter_ctx, c, 1);
+return ssl_filter_io_shutdown(filter_ctx, c, 1, 1);
}
}
@@ -1155,7 +1159,7 @@
ap_log_error(APLOG_MARK, APLOG_INFO, 0, c-base_server,
No acceptable peer certificate available);
-return ssl_filter_io_shutdown(filter_ctx, c, 1);
+return ssl_filter_io_shutdown(filter_ctx, c, 1, 1);
}
return APR_SUCCESS;
@@ -1392,6 +1396,7 @@
/* bio_filter_out_flush() already passed down
a flush bucket
* if there was any data to be flushed.
*/
+ssl_filter_io_shutdown(filter_ctx, f-c, 0, 0);
apr_bucket_delete(bucket);
}
}
Re: [PATCH] SSL not sending close alert message
On Mon, Feb 23, 2004 at 01:22:05PM -0800, Mathihalli, Madhusudan wrote: Hi, I started working on Justin's idea of creating a EOC bucket - to do a SSL shutdown before the socket close(). But since the ap_flush_conn is called just before closing the socket - I thought of doing the SSL shutdown during the flush itself. Let me know what you think of this patch. This is just back to what we had patches for already: doing an SSL shutdown on any EOF bucket, right? Which is not right since you get an EOS after each HTTP response, not at the end of the connection. Hence the need for a new bucket type to represent end-of-connection differently from EOS. (the test case for that is to see if you can send two requests on a single SSL connection) joe
RE: [PATCH] SSL not sending close alert message
When I did a ssldump to analyze the default index.html access (using MSIE 6.0), it showed that there were 3 requests and 3 connections - the session-id stayed the same. (dumb question: ) How did you manage the 2 requests on the same connection ? -Madhu -Original Message- From: Joe Orton [mailto:[EMAIL PROTECTED] Sent: Monday, February 23, 2004 2:07 PM To: [EMAIL PROTECTED] Subject: Re: [PATCH] SSL not sending close alert message On Mon, Feb 23, 2004 at 01:22:05PM -0800, Mathihalli, Madhusudan wrote: Hi, I started working on Justin's idea of creating a EOC bucket - to do a SSL shutdown before the socket close(). But since the ap_flush_conn is called just before closing the socket - I thought of doing the SSL shutdown during the flush itself. Let me know what you think of this patch. This is just back to what we had patches for already: doing an SSL shutdown on any EOF bucket, right? Which is not right since you get an EOS after each HTTP response, not at the end of the connection. Hence the need for a new bucket type to represent end-of-connection differently from EOS. (the test case for that is to see if you can send two requests on a single SSL connection) joe
RE: [PATCH] SSL not sending close alert message
Oh.. forget it - it was some setting in my browser :( Thanks -Madhu From: Mathihalli, Madhusudan Sent: Mon 2/23/2004 3:16 PM To: [EMAIL PROTECTED] Subject: RE: [PATCH] SSL not sending close alert message When I did a ssldump to analyze the default index.html access (using MSIE 6.0), it showed that there were 3 requests and 3 connections - the session-id stayed the same. (dumb question: ) How did you manage the 2 requests on the same connection ? -Madhu -Original Message- From: Joe Orton [mailto:[EMAIL PROTECTED] Sent: Monday, February 23, 2004 2:07 PM To: [EMAIL PROTECTED] Subject: Re: [PATCH] SSL not sending close alert message On Mon, Feb 23, 2004 at 01:22:05PM -0800, Mathihalli, Madhusudan wrote: Hi, I started working on Justin's idea of creating a EOC bucket - to do a SSL shutdown before the socket close(). But since the ap_flush_conn is called just before closing the socket - I thought of doing the SSL shutdown during the flush itself. Let me know what you think of this patch. This is just back to what we had patches for already: doing an SSL shutdown on any EOF bucket, right? Which is not right since you get an EOS after each HTTP response, not at the end of the connection. Hence the need for a new bucket type to represent end-of-connection differently from EOS. (the test case for that is to see if you can send two requests on a single SSL connection) joe winmail.dat
Re: [PATCH] SSL not sending close alert message
At 04:07 PM 2/23/2004, Joe Orton wrote: On Mon, Feb 23, 2004 at 01:22:05PM -0800, Mathihalli, Madhusudan wrote: Hi, I started working on Justin's idea of creating a EOC bucket - to do a SSL shutdown before the socket close(). But since the ap_flush_conn is called just before closing the socket - I thought of doing the SSL shutdown during the flush itself. Let me know what you think of this patch. This is just back to what we had patches for already: doing an SSL shutdown on any EOF bucket, right? Which is not right since you get an EOS after each HTTP response, not at the end of the connection. Hence the need for a new bucket type to represent end-of-connection differently from EOS. Do we? I suspect that if the http protocol filter knew the difference between keep alive and connection close requests, it should eat non-terminal EOS marks (and pass flush instead?) while still passing a final EOS to the network stack layer? Bill
Re: [PATCH] SSL not sending close alert message
On Tue, 24 Feb 2004, William A. Rowe, Jr. wrote: I suspect that if the http protocol filter knew the difference between keep alive and connection close requests, it should eat non-terminal EOS marks (and pass flush instead?) while still passing a final EOS to the network stack layer? That would break a lot of stuff :)
