Re: [PATCH] rev. 2 of my apache2filters SEGV fixes

Aaron Bannert wrote:

I posted this patch earlier, but this has some improvements. First of all, it's tabified, so we have consistent style. It also has some performance improvements suggested by Cliff Woolley, including one that will significantly improve output buffering/copying. The main guts of the patch include a overhaul/rewrite of the php_output_filter. It no longer requires setaside brigades, and can handle exactly one FILE bucket (Which it promptly passes off to PHP for processing). If it finds no FILE bucket, it silently passes the brigade bits down to the next filter. After processing a FILE, it steps out of the way and passes all other brigades down the chain. The SEGVs reported earlier on this list and others are fixed with this patch, as well as some other potential problems. -aaron p.s. this patch applies to the php4_2_0RC2 branch, and includes some changes from HEAD that weren't yet merged into that branch, namly the changes to use the new bucket allocator. Index: sapi/apache2filter/sapi_apache2.c =================================================================== RCS file: /repository/php4/sapi/apache2filter/sapi_apache2.c,v retrieving revision 1.61.2.1 diff -u -u -r1.61.2.1 sapi_apache2.c --- sapi/apache2filter/sapi_apache2.c 14 Mar 2002 10:57:00 -0000 1.61.2.1 +++ sapi/apache2filter/sapi_apache2.c 10 Apr 2002 20:36:27 -0000 @@ -48,26 +48,31 @@ { apr_bucket *b; apr_bucket_brigade *bb; + apr_bucket_alloc_t *ba; php_struct *ctx; - uint now; ctx = SG(server_context); if (str_length == 0) return 0; - bb = apr_brigade_create(ctx->f->r->pool); - while (str_length > 0) { - now = MIN(str_length, 4096); - b = apr_bucket_transient_create(str, now); - APR_BRIGADE_INSERT_TAIL(bb, b); - str += now; - str_length -= now; - } + ba = ctx->f->c->bucket_alloc; + bb = apr_brigade_create(ctx->f->r->pool, ba); + + b = apr_bucket_transient_create(str, str_length, ba); + APR_BRIGADE_INSERT_TAIL(bb, b); + + /* Add a Flush bucket to the end of this brigade, so that + * the transient buckets above are more likely to make it out + * the end of the filter instead of having to be copied into + * someone's setaside. */ + b = apr_bucket_flush_create(ba); + APR_BRIGADE_INSERT_TAIL(bb, b); + if (ap_pass_brigade(ctx->f->next, bb) != APR_SUCCESS) { php_handle_aborted_connection(); } - return str_length; + return 0; /* we wrote everything, we promise! */

Why did you change this to return 0? It breaks ImageJpeg (and likely other things) as GD checks that the return value matches the passed length & aborts if it doesn't. Otherwise, this patch seems to work much better than the HEAD code.