You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by do...@locus.apache.org on 2000/09/11 06:33:31 UTC
cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
dougm 00/09/10 21:33:31
Modified: src/ap ap_buckets_mmap.c
Log:
if a filter fumbles and does not send anything downstream, data == NULL == SEGV
Revision Changes Path
1.8 +4 -0 apache-2.0/src/ap/ap_buckets_mmap.c
Index: ap_buckets_mmap.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/ap/ap_buckets_mmap.c,v
retrieving revision 1.7
retrieving revision 1.8
diff -u -r1.7 -r1.8
--- ap_buckets_mmap.c 2000/08/19 16:54:45 1.7
+++ ap_buckets_mmap.c 2000/09/11 04:33:31 1.8
@@ -77,6 +77,10 @@
{
ap_bucket_mmap *m;
+ if (data == NULL) {
+ return;
+ }
+
m = ap_bucket_destroy_shared(data);
if (m == NULL) {
return;
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by Tony Finch <do...@dotat.at>.
Doug MacEachern <do...@covalent.net> wrote:
>
>maybe, but that routine already does a similar check:
> m = ap_bucket_destroy_shared(data);
> if (m == NULL) {
> return;
> }
>
>i don't see a problem with another sanity check there. if there's a
>better fix i'd be happy to see it.
That's not a sanity check, it's part of the API. If the refcount
indicated that the underlying object is still in use, destroy_shared
returns NULL, otherwise it returns a pointer to the object to be freed.
Tony.
--
en oeccget g mtcaa f.a.n.finch
v spdlkishrhtewe y dot@dotat.at
eatp o v eiti i d. fanf@covalent.net
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by rb...@covalent.net.
> > I believe the ability for a bucket to have NULL data was introduced with
> > the BRIGADE macros. I ran into similar problems last night while trying
> > to finish mod_include. The problem with the brigade macros, is that you
> > must use the macros to do any looping through a brigade or bucket list.
>
> Umm. Are you saying there is a problem in the brigade macros? That they
> create buckets with NULL data fields?
>
> I'm unclear on what you're saying here.
Sorry. All I meant was that if you try to iterate through the brigades
without using the macros, things WILL break. I haven't looked at the
macros in any great detail to really understand why, but they break. As
long as you use the macros, you are fine though.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Sep 11, 2000 at 12:38:07PM -0700, rbb@covalent.net wrote:
>...
> > But the underlying question is how bucket.data became NULL. Whether or not
> > the data is passed, I don't understand that part.
> >
> > I just saw that you reversed this change; did you find a problem in your
> > code which caused data==NULL?
>
> I believe the ability for a bucket to have NULL data was introduced with
> the BRIGADE macros. I ran into similar problems last night while trying
> to finish mod_include. The problem with the brigade macros, is that you
> must use the macros to do any looping through a brigade or bucket list.
Umm. Are you saying there is a problem in the brigade macros? That they
create buckets with NULL data fields?
I'm unclear on what you're saying here.
thx,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by Doug MacEachern <do...@covalent.net>.
On Mon, 11 Sep 2000 rbb@covalent.net wrote:
> I believe the ability for a bucket to have NULL data was introduced with
> the BRIGADE macros. I ran into similar problems last night while trying
> to finish mod_include. The problem with the brigade macros, is that you
> must use the macros to do any looping through a brigade or bucket list.
exactly, i was following mod_charset_lite as an example, which i later
realilzed hadn't been updated to use the new macros. switching my test to
use logic from http_core filter made things much happier.
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by rb...@covalent.net.
>
> I agree that it can/should be possible to skip ap_pass_brigade. It is
> possible that a filter will decide not to pass some content down (e.g. say
> there is a conditional wrapped around some content).
If a filter doesn't pass down any data, then it should just be returning
to the previous filter. At some point, the filter must pass data down, or
there is nothing for lower level filters to act on.
>
> But the underlying question is how bucket.data became NULL. Whether or not
> the data is passed, I don't understand that part.
>
> I just saw that you reversed this change; did you find a problem in your
> code which caused data==NULL?
I believe the ability for a bucket to have NULL data was introduced with
the BRIGADE macros. I ran into similar problems last night while trying
to finish mod_include. The problem with the brigade macros, is that you
must use the macros to do any looping through a brigade or bucket list.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by Doug MacEachern <do...@covalent.net>.
On Mon, 11 Sep 2000, Greg Stein wrote:
> But the underlying question is how bucket.data became NULL. Whether or not
> the data is passed, I don't understand that part.
>
> I just saw that you reversed this change; did you find a problem in your
> code which caused data==NULL?
i backed it out because two people who know filtering a whole lot better
than me didn't like it :) the segv is also gone without sending data
downstream. i think the real problem was my test filter not properly
using the new AP_{BRIGADE,BUCKET} macros, sorry for the foncusion.
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by Greg Stein <gs...@lyra.org>.
On Mon, Sep 11, 2000 at 11:00:26AM -0700, Doug MacEachern wrote:
> On Mon, 11 Sep 2000, Greg Stein wrote:
>
> > I don't understand this one. How can bucket.data end up as NULL? AFAIK, that
> > just shouldn't happen.
>
> as i said in the commit message, it happens if a filter doesn't send any
> data downstream. i'm just starting to play with filtering, i had a
> mod_test_filter.c with a:
> apr_status_t filter_handler(ap_filter_t *f, ap_bucket_brigade *bb)
>
> that did not call ap_pass_brigade(), or do anything for that matter.
> yeah, that shouldn't happen, but it can and dumb filters shouldn't be able
> to make apache segv.
I agree that it can/should be possible to skip ap_pass_brigade. It is
possible that a filter will decide not to pass some content down (e.g. say
there is a conditional wrapped around some content).
But the underlying question is how bucket.data became NULL. Whether or not
the data is passed, I don't understand that part.
I just saw that you reversed this change; did you find a problem in your
code which caused data==NULL?
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by Jeff Trawick <tr...@bellsouth.net>.
Doug MacEachern <do...@covalent.net> writes:
> On Mon, 11 Sep 2000, Greg Stein wrote:
>
> > I don't understand this one. How can bucket.data end up as NULL? AFAIK, that
> > just shouldn't happen.
>
> as i said in the commit message, it happens if a filter doesn't send any
> data downstream. i'm just starting to play with filtering, i had a
> mod_test_filter.c with a:
> apr_status_t filter_handler(ap_filter_t *f, ap_bucket_brigade *bb)
>
> that did not call ap_pass_brigade(), or do anything for that matter.
> yeah, that shouldn't happen, but it can and dumb filters shouldn't be able
> to make apache segv.
We can't keep dumb module code from making apache segv without
introducing a lot of overhead (to run module code in a sandbox).
If it were my module, I'd rather it segv. I'd find/fix the bug much
quicker that way.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by Doug MacEachern <do...@covalent.net>.
On Mon, 11 Sep 2000, Greg Stein wrote:
> I don't understand this one. How can bucket.data end up as NULL? AFAIK, that
> just shouldn't happen.
as i said in the commit message, it happens if a filter doesn't send any
data downstream. i'm just starting to play with filtering, i had a
mod_test_filter.c with a:
apr_status_t filter_handler(ap_filter_t *f, ap_bucket_brigade *bb)
that did not call ap_pass_brigade(), or do anything for that matter.
yeah, that shouldn't happen, but it can and dumb filters shouldn't be able
to make apache segv.
> The patch below seems to only fix the symptom, not the original cause.
maybe, but that routine already does a similar check:
m = ap_bucket_destroy_shared(data);
if (m == NULL) {
return;
}
i don't see a problem with another sanity check there. if there's a
better fix i'd be happy to see it.
Re: cvs commit: apache-2.0/src/ap ap_buckets_mmap.c
Posted by Greg Stein <gs...@lyra.org>.
I don't understand this one. How can bucket.data end up as NULL? AFAIK, that
just shouldn't happen.
The patch below seems to only fix the symptom, not the original cause.
Cheers,
-g
On Mon, Sep 11, 2000 at 04:33:31AM -0000, dougm@locus.apache.org wrote:
> dougm 00/09/10 21:33:31
>
> Modified: src/ap ap_buckets_mmap.c
> Log:
> if a filter fumbles and does not send anything downstream, data == NULL == SEGV
>
> Revision Changes Path
> 1.8 +4 -0 apache-2.0/src/ap/ap_buckets_mmap.c
>
> Index: ap_buckets_mmap.c
> ===================================================================
> RCS file: /home/cvs/apache-2.0/src/ap/ap_buckets_mmap.c,v
> retrieving revision 1.7
> retrieving revision 1.8
> diff -u -r1.7 -r1.8
> --- ap_buckets_mmap.c 2000/08/19 16:54:45 1.7
> +++ ap_buckets_mmap.c 2000/09/11 04:33:31 1.8
> @@ -77,6 +77,10 @@
> {
> ap_bucket_mmap *m;
>
> + if (data == NULL) {
> + return;
> + }
> +
> m = ap_bucket_destroy_shared(data);
> if (m == NULL) {
> return;
>
>
>
--
Greg Stein, http://www.lyra.org/