-------- Original-Nachricht --------
Betreff:        svn commit: r1197405 - in /httpd/httpd/trunk: CHANGES 
docs/manual/upgrading.xml modules/filters/mod_substitute.c
Datum:  Fri, 04 Nov 2011 05:35:54 GMT
Von:    s...@apache.org



Author: sf
Date: Fri Nov  4 05:35:53 2011
New Revision: 1197405

URL: http://svn.apache.org/viewvc?rev=1197405&view=rev
Log:
To prevent overboarding memory usage, limit line length to 1MB

Modified:
    httpd/httpd/trunk/CHANGES
    httpd/httpd/trunk/docs/manual/upgrading.xml
    httpd/httpd/trunk/modules/filters/mod_substitute.c

Modified: httpd/httpd/trunk/modules/filters/mod_substitute.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_substitute.c?rev=1197405&r1=1197404&r2=1197405&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/filters/mod_substitute.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_substitute.c Fri Nov  4 05:35:53 2011
@@ -158,6 +164,9 @@ static void do_pattmatch(ap_filter_t *f,
                              * as its own bucket, then isolate the pattern
                              * and delete it.
                              */
+                            if (space_left<  len + repl_len)
+                                return APR_ENOMEM;
+                            space_left -= len + repl_len;


Why is this needed at all? IMHO we only consume memory by creating additional 
bucket structures,
but this does not depend on the length of the replacement strings.
As we use transient buckets below we use the same data (script->replacement) 
over and over
again for new buckets.

                             SEDRMPATBCKT(b, len, tmp_b, script->patlen);
                             /*
                              * Finally, we create a bucket that contains the
@@ -188,25 +197,36 @@ static void do_pattmatch(ap_filter_t *f,
                     int left = bytes;
                     const char *pos = buff;
                     char *repl;
+                    apr_size_t space_left = AP_SUBST_MAX_LINE_LENGTH;
                     while (!ap_regexec_len(script->regexp, pos, left,
                                        AP_MAX_REG_MATCH, regm, 0)) {
+                        apr_status_t rv;
                         have_match = 1;
                         if (script->flatten&&  !force_quick) {
                             /* copy bytes before the match */
                             if (regm[0].rm_so>  0)
                                 ap_varbuf_strmemcat(&vb, pos, regm[0].rm_so);
                             /* add replacement string */
-                            ap_varbuf_regsub(&vb, script->replacement, pos,
-                                             AP_MAX_REG_MATCH, regm, 0);
+                            rv = ap_varbuf_regsub(&vb, script->replacement, 
pos,
+                                                  AP_MAX_REG_MATCH, regm,
+                                                  AP_SUBST_MAX_LINE_LENGTH - 
vb.strlen);
+                            if (rv != APR_SUCCESS)
+                                return rv;
                         }
                         else {
-                            ap_pregsub_ex(pool,&repl, script->replacement, pos,
-                                              AP_MAX_REG_MATCH, regm, 0);
+                            apr_size_t repl_len;
+                            rv = ap_pregsub_ex(pool,&repl,
+                                               script->replacement, pos,
+                                               AP_MAX_REG_MATCH, regm,
+                                               space_left);
+                            if (rv != APR_SUCCESS)
+                                return rv;
                             len = (apr_size_t) (regm[0].rm_eo - regm[0].rm_so);
+                            repl_len = strlen(repl);
+                            space_left -= len + repl_len;


Isn't it sufficient to reduce space_left only by repl_len?
IMHO we only allocate repl_len new memory in ap_pregsub_ex and not len + 
repl_len or am I wrong here?



                             SEDRMPATBCKT(b, regm[0].rm_so, tmp_b, len);
-                            tmp_b = apr_bucket_transient_create(repl,
-                                             strlen(repl),
-                                             f->r->connection->bucket_alloc);
+                            tmp_b = apr_bucket_transient_create(repl, repl_len,
+                                                
f->r->connection->bucket_alloc);
                             APR_BUCKET_INSERT_BEFORE(b, tmp_b);
                         }
                         /*

@@ -414,9 +450,8 @@ static apr_status_t substitute_filter(ap
                             rv = ap_pass_brigade(f->next, ctx->passbb);
                             apr_brigade_cleanup(ctx->passbb);
                             num = 0;
-                            apr_pool_clear(ctx->tpool);



Why don't we clear this pool any longer? It was cleared in both cases:
The error one and the none error one.
And calling  apr_pool_clear twice does not harm.


                             if (rv != APR_SUCCESS)
-                                return rv;
+                                goto err;
                         }
                         b = tmp_b;
                     }


Regards

Rüdiger

Reply via email to