You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "Mathihalli, Madhusudan" <ma...@hp.com> on 2004/02/25 02:26:03 UTC
[PATCH-Modified] SSL not sending close alert message
Here's the new patch incorporating the feedback.
One thing that I'd like feedback : The eoc bucket is inserted by the ap_end_connection() function(in server/connection.c) - and deleted by SSL in ssl_engine_io.c. I don't feel comfortable with it.
Is that okay ? If not, what is a good place for inserting the EOC bucket (in mod_ssl) ?
Thanks
-Madhu
Index: server/connection.c
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/connection.c,v
retrieving revision 1.114
diff -u -r1.114 connection.c
--- server/connection.c 9 Feb 2004 20:40:49 -0000 1.114
+++ server/connection.c 25 Feb 2004 00:41:18 -0000
@@ -65,10 +65,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 = ap_bucket_eoc_create(c->bucket_alloc);
+ APR_BRIGADE_INSERT_TAIL(bb, b);
+ ap_pass_brigade(c->output_filters, bb);
+}
+
AP_CORE_DECLARE(void) ap_flush_conn(conn_rec *c)
{
apr_bucket_brigade *bb;
apr_bucket *b;
+
+ ap_end_connection(c);
bb = apr_brigade_create(c->pool, c->bucket_alloc);
b = apr_bucket_flush_create(c->bucket_alloc);
Index: include/http_protocol.h
===================================================================
RCS file: /home/cvspublic/httpd-2.0/include/http_protocol.h,v
retrieving revision 1.87
diff -u -r1.87 http_protocol.h
--- include/http_protocol.h 26 Jan 2004 22:08:06 -0000 1.87
+++ include/http_protocol.h 25 Feb 2004 00:42:48 -0000
@@ -666,6 +666,32 @@
*/
AP_DECLARE_HOOK(apr_port_t,default_port,(const request_rec *r))
+AP_DECLARE_DATA extern const apr_bucket_type_t ap_bucket_type_eoc;
+
+/**
+ * Determine if a bucket is an End Of Connection (EOC) bucket
+ * @param e The bucket to inspect
+ * @return true or false
+ */
+#define AP_BUCKET_IS_EOC(e) (e->type == &ap_bucket_type_eoc)
+
+/**
+ * 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
+ * @deffunc apr_bucket *ap_bucket_eoc_make(apr_bucket *b)
+ */
+AP_DECLARE(apr_bucket *) ap_bucket_eoc_make(apr_bucket *b);
+
+/**
+ * Create a bucket referring to an End Of Connection (EOC). 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
+ * @deffunc apr_bucket *ap_bucket_eoc_create(apr_bucket_alloc_t *list)
+ */
+AP_DECLARE(apr_bucket *) ap_bucket_eoc_create(apr_bucket_alloc_t *list);
+
typedef struct ap_bucket_error ap_bucket_error;
/**
Index: modules/ssl/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
--- modules/ssl/ssl_engine_io.c 9 Feb 2004 20:29:22 -0000 1.117
+++ modules/ssl/ssl_engine_io.c 25 Feb 2004 01:16:28 -0000
@@ -100,6 +100,7 @@
BIO *pbioWrite;
ap_filter_t *pInputFilter;
ap_filter_t *pOutputFilter;
+ int nobuffer; /* non-zero to prevent buffering */
} ssl_filter_ctx_t;
typedef struct {
@@ -193,7 +194,8 @@
*/
BIO_clear_retry_flags(bio);
- if (!outctx->length && (inl + outctx->blen < sizeof(outctx->buffer))) {
+ if (!outctx->length && (inl + outctx->blen < sizeof(outctx->buffer)) &&
+ !outctx->filter_ctx->nobuffer) {
/* the first two SSL_writes (of 1024 and 261 bytes)
* need to be in the same packet (vec[0].iov_base)
*/
@@ -1394,6 +1396,14 @@
*/
apr_bucket_delete(bucket);
}
+ }
+ else if (AP_BUCKET_IS_EOC(bucket)) {
+ /* The special "EOC" bucket means a shutdown is needed;
+ * turn off buffering in bio_filter_out_write.
+ */
+ filter_ctx->nobuffer = 1;
+ status = ssl_filter_io_shutdown(filter_ctx, f->c, 0);
+ apr_bucket_delete(bucket);
}
else {
/* filter output */
Index: server/Makefile.in
===================================================================
RCS file: /home/cvspublic/httpd-2.0/server/Makefile.in,v
retrieving revision 1.91
diff -u -r1.91 Makefile.in
--- server/Makefile.in 2 Feb 2004 17:04:10 -0000 1.91
+++ server/Makefile.in 25 Feb 2004 00:44:05 -0000
@@ -13,7 +13,8 @@
connection.c listen.c \
mpm_common.c util_charset.c util_debug.c util_xml.c \
util_filter.c exports.c buildmark.c \
- scoreboard.c error_bucket.c protocol.c core.c request.c provider.c
+ scoreboard.c error_bucket.c protocol.c core.c request.c provider.c \
+ eoc_bucket.c
TARGETS = delete-exports $(LTLIBRARY_NAME) $(CORE_IMPLIB_FILE) export_vars.h httpd.exp
------------------------------------------------------------------------------
NEW FILE: server/eoc_bucket.c
-----------------------------------------------------------------------------
/* Copyright 2000-2004 The Apache Software Foundation
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
#include "http_protocol.h"
#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 *) ap_bucket_eoc_make(apr_bucket *b)
{
b->length = 0;
b->start = 0;
b->data = NULL;
b->type = &ap_bucket_type_eoc;
return b;
}
APU_DECLARE(apr_bucket *) ap_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 ap_bucket_eoc_make(b);
}
APU_DECLARE_DATA const apr_bucket_type_t ap_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
};
Re: [PATCH-Modified] SSL not sending close alert message
Posted by Justin Erenkrantz <ju...@erenkrantz.com>.
--On Tuesday, February 24, 2004 5:26 PM -0800 "Mathihalli, Madhusudan"
<ma...@hp.com> wrote:
> Here's the new patch incorporating the feedback.
Comments inline.
> One thing that I'd like feedback : The eoc bucket is inserted by the
> ap_end_connection() function(in server/connection.c) - and deleted by SSL
> in ssl_engine_io.c. I don't feel comfortable with it.
>
> Is that okay ? If not, what is a good place for inserting the EOC bucket (in
> mod_ssl) ?
Agreed. It needs to be passed on to the next filter as well.
> Index: server/connection.c
> ===================================================================
> RCS file: /home/cvspublic/httpd-2.0/server/connection.c,v
> retrieving revision 1.114
> diff -u -r1.114 connection.c
> --- server/connection.c 9 Feb 2004 20:40:49 -0000 1.114
> +++ server/connection.c 25 Feb 2004 00:41:18 -0000
> @@ -65,10 +65,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 = ap_bucket_eoc_create(c->bucket_alloc);
> + APR_BRIGADE_INSERT_TAIL(bb, b);
> + ap_pass_brigade(c->output_filters, bb);
> +}
> +
> AP_CORE_DECLARE(void) ap_flush_conn(conn_rec *c)
> {
> apr_bucket_brigade *bb;
> apr_bucket *b;
> +
> + ap_end_connection(c);
I wouldn't split this out into two functions that both create a brigade and
pass it. I'd just create the EOC bucket and stick it *after* the flush bucket
(see below for why I think that ordering makes sense).
> bb = apr_brigade_create(c->pool, c->bucket_alloc);
> b = apr_bucket_flush_create(c->bucket_alloc);
...
> Index: modules/ssl/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
> --- modules/ssl/ssl_engine_io.c 9 Feb 2004 20:29:22 -0000 1.117
> +++ modules/ssl/ssl_engine_io.c 25 Feb 2004 01:16:28 -0000
> @@ -100,6 +100,7 @@
> BIO *pbioWrite;
> ap_filter_t *pInputFilter;
> ap_filter_t *pOutputFilter;
> + int nobuffer; /* non-zero to prevent buffering */
> } ssl_filter_ctx_t;
>
> typedef struct {
> @@ -193,7 +194,8 @@
> */
> BIO_clear_retry_flags(bio);
>
> - if (!outctx->length && (inl + outctx->blen < sizeof(outctx->buffer))) {
> + if (!outctx->length && (inl + outctx->blen < sizeof(outctx->buffer)) &&
> + !outctx->filter_ctx->nobuffer) {
> /* the first two SSL_writes (of 1024 and 261 bytes)
> * need to be in the same packet (vec[0].iov_base)
> */
> @@ -1394,6 +1396,14 @@
> */
> apr_bucket_delete(bucket);
> }
> + }
> + else if (AP_BUCKET_IS_EOC(bucket)) {
> + /* The special "EOC" bucket means a shutdown is needed;
> + * turn off buffering in bio_filter_out_write.
> + */
> + filter_ctx->nobuffer = 1;
> + status = ssl_filter_io_shutdown(filter_ctx, f->c, 0);
> + apr_bucket_delete(bucket);
Actually, I don't think we need nobuffer. Or, could we end up being
recursively called when we call shutdown? (I forget the call ordering here.)
Our contract can state that there will be no more data after an EOC. This is
much the same as there should be no data after an EOS to a request-level
filter.
I would also not delete this bucket, I'd just try to leave it there to pass on
to the next filter. I'd want the core_conn_filter to see the EOC as well.
That *might* mean the ordering should be FLUSH *then* EOC instead of EOC
*then* FLUSH (as you have it now).
Thanks! -- justin