Re: trick/tips for finding memory leaks
On Sun, 25 Nov 2007 20:16:00 -0500 Sam Carleton [EMAIL PROTECTED] wrote: Thanks to the performance tools of my OS I have confirmed that somewhere in my Apache module there is a memory leak. Are there any tips or tricks out there for find memory leaks in an Apache module? Run inside gdb as a single process, not forking any background processes, -X i think. Then step through your code to see where the leak is. Generally speaking, if you're using apr routines then they should handle the memory resources for you, there should be little need for allocating on the heap. -- The 28.8 frame relay to the Netapp is smelling funky because of All your base are belong to us. Sun Microsystems is havin' a smoke. :: http://www.s5h.net/ :: http://www.s5h.net/gpg.html signature.asc Description: PGP signature
Re: performance vs development time
On Sun, 25 Nov 2007 21:32:13 -0500 Sam Carleton [EMAIL PROTECTED] wrote: With this memory leak in my simple Apache module, I am considering rewriting the whole module. Right now there are two files small files that the module reads every time. One is a small (less then a 1K) configuration file and the other is a small (1K ~20K) xml file. In the rewrite, I am considering caching the data in these files and reading them only if they are changed. The question though is: Considering how small these files are, will the performance gains be worth the extra development time? Another option would be to switch from using libxml2 to expat for the XML parsing. Oh, what type of load is the server under? The server is driving a kiosk system where there are normally a hand full of kiosk but there could be as many as 100 under very heavy use. If it's a kiosk system then it's not really going to matter how you're processing that as the main bottle neck is a human, so you can probably ignore the fact that it's in memory cache. If you want to store the file in memory though, put a void * in your module's .c file, outside of the module routines, so that it's defined at initialisation. Then in your init routines allocate the memory for it. The trouble that you might find yourself in is how to lock this pointer when the file changes AND the memory is being read/written by concurrent processes. For this reason I suggest that you stick to reading the data on access from disk. If disk IO is a problem then put the file on the RAM disk, where there is going to be little difference between having the data read at module run time or from memory - some file system operations are atomic, so you don't need to worry so much. HTH. -- The 28.8 frame relay to south lata is unreliable because of Bernard Shifman threatening to sue. RedHat is selling their dialup customers to Earthlink. :: http://www.s5h.net/ :: http://www.s5h.net/gpg.html signature.asc Description: PGP signature
Re: performance vs development time
Am Sonntag, den 25.11.2007, 21:32 -0500 schrieb Sam Carleton: In the rewrite, I am considering caching the data in these files and reading them only if they are changed. The question though is: Considering how small these files are, will the performance gains be worth the extra development time? No. A decent operating system will keep the files in memory unless they are changed anyway. You could have the same performance gain with a 100 $s worth of CPU or a faster disk. In addition all extra code reduces reliability no matter how good it is. Another option would be to switch from using libxml2 to expat for the XML parsing. Depends. libxml2 has much more functionality, especially a DOM implementation. If you are only doing SAX, expat may be an option. I doubt however that it will give you a speed gain. Sincerely, Joachim
Re: trick/tips for finding memory leaks
On 11/26/07, ed [EMAIL PROTECTED] wrote: Generally speaking, if you're using apr routines then they should handle the memory resources for you, there should be little need for allocating on the heap.
Expression Parser API?
mod_include has an expression parser (parse_expr at line 1125 in /trunk/). Many other modules implement simpler parsers for a range of purposes. It seems to me we could potentially benefit from a general- purpose expression parser, and I'm wondering about extracting mod_include's parse_expr as ap_parse_expr, maybe in util.c. The job looks like mostly one of removing the include_ctx and substituting request_rec, or maybe even pool and a void*. Does that make sense? Thoughts? -- Nick Kew Application Development with Apache - the Apache Modules Book http://www.apachetutor.org/
Re: Expression Parser API?
On Mon, November 26, 2007 4:18 pm, Nick Kew wrote: mod_include has an expression parser (parse_expr at line 1125 in /trunk/). Many other modules implement simpler parsers for a range of purposes. It seems to me we could potentially benefit from a general- purpose expression parser, and I'm wondering about extracting mod_include's parse_expr as ap_parse_expr, maybe in util.c. The job looks like mostly one of removing the include_ctx and substituting request_rec, or maybe even pool and a void*. Does that make sense? Thoughts? It makes sense. I think a worthwhile exercise is to go through the code and identify possibly duplicated code, or code with general application to the server at large that could be used by other modules, and make this code common. Regards, Graham --
Re: svn commit: r595954 - /httpd/httpd/trunk/modules/http/http_filters.c
On Nov 17, 2007 9:36 AM, [EMAIL PROTECTED] wrote:
Author: niq
Date: Sat Nov 17 06:36:58 2007
New Revision: 595954
URL: http://svn.apache.org/viewvc?rev=595954view=rev
Log:
Safer fix to PR43882 than in r595672.
Modified:
httpd/httpd/trunk/modules/http/http_filters.c
Modified: httpd/httpd/trunk/modules/http/http_filters.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http/http_filters.c?rev=595954r1=595953r2=595954view=diff
==
--- httpd/httpd/trunk/modules/http/http_filters.c (original)
+++ httpd/httpd/trunk/modules/http/http_filters.c Sat Nov 17 06:36:58 2007
@@ -115,11 +115,11 @@
lenp = apr_table_get(f-r-headers_in, Content-Length);
if (tenc) {
-/* RFC2616 allows qualifiers, so use strncasecmp */
-if (!strncasecmp(tenc, chunked, 7) !ap_strchr_c(tenc, ','))
{
+if (!strcasecmp(tenc, chunked)) {
ctx-state = BODY_CHUNK;
}
-else {
+ /* test lenp, because it gives another case we can handle */
+else if (!lenp) {
/* Something that isn't in HTTP, unless some future
* edition defines new transfer ecodings, is unsupported.
The overall flow now looks like:
if Transfer-Encoding {
if Transfer-Encoding == chunked {
respect it
}
else if no Content-Length {
log it and return an error
}
else {
UNHANDLED case with unrecognized Transfer-Encoding as well as
Content-Length
}
}
else if Content-Length {
respect CL
}
What is the intention for the UNHANDLED case?The code/comments
seem to imply we'll end up in the respect CL path.
Re: svn commit: r595954 - /httpd/httpd/trunk/modules/http/http_filters.c
On Mon, 26 Nov 2007 10:38:28 -0500 Jeff Trawick [EMAIL PROTECTED] wrote: What is the intention for the UNHANDLED case?The code/comments seem to imply we'll end up in the respect CL path. Exactly. The alternative is to reject it, which might risk breaking something that worked before. The current fix falls back to old behaviour in the have-CL-bad-TE case, so there's no risk of regression. What else do you think we should do in that case? Perhaps logging an error would be a good idea. -- Nick Kew Application Development with Apache - the Apache Modules Book http://www.apachetutor.org/
Appologies: httpd/httpd/vendor/ SNAFU
Hi All,
I accidentally committed an upgrade to httpd/httpd/vendor/pcre/current
to 7.4. I apparently had a commit bit there because I'm on the PMC from
past apreq work.
I immediately asked what to do over on #infra on freenode and jerenkrantz
agreed I should back it out so I did.
It was committed in r598339 and removed in r598343.
I was following th README there and did not execute the svn cp after
actually seing the ci complete. I was expecting it to fail.
httpd/httpd/{branches,trunk} were not affected.
If I need to do anything else to undo it let me know.
I shall be more careful in the future.
On another note, the reasoning behind this is FreeBSD ports supposed
WITH_PCRE_FROM_PORTS option in www/apache22 et al. Which works flawlessly
so I thought it might be a good idea; however, I definetely wanted it
reviewed on this list first.
Again, my appologies.
--
Philip M. Gollucci ([EMAIL PROTECTED]) 323.219.4708
Senior System Admin - Riderway, Inc. http://riderway.com
1024D/EC88A0BF 0DE5 C55C 6BF3 B235 2DAB B89E 1324 9B4F EC88 A0BF
Work like you don't need the money,
love like you'll never get hurt,
and dance like nobody's watching.
Re: svn commit: r595954 - /httpd/httpd/trunk/modules/http/http_filters.c
On Nov 26, 2007 11:50 AM, Nick Kew [EMAIL PROTECTED] wrote:
On Mon, 26 Nov 2007 10:38:28 -0500
Jeff Trawick [EMAIL PROTECTED] wrote:
What is the intention for the UNHANDLED case?The code/comments
seem to imply we'll end up in the respect CL path.
Exactly.
Cool; we're in sync so far, which brings us to my concern: We won't
end up in the respect CL path ;)
Finding any Transfer-Encoding, supported or not, results in bypassing
this support for CL:
else if (lenp) {
char *endstr;
ctx-state = BODY_LENGTH;
errno = 0;
[review] upgrade pcre from 6.7 - 7.4 for httpd/trunk
I figured after the earlier snafu, I should at least send this to the list for review. I won't do anything with it unless people think its a good thing. -- Philip M. Gollucci ([EMAIL PROTECTED]) 323.219.4708 Senior System Admin - Riderway, Inc. http://riderway.com 1024D/EC88A0BF 0DE5 C55C 6BF3 B235 2DAB B89E 1324 9B4F EC88 A0BF Work like you don't need the money, love like you'll never get hurt, and dance like nobody's watching. pcre.svn Description: Binary data
Re: svn commit: r598299 - in /httpd/httpd/trunk: CHANGES modules/filters/mod_filter.c
On 11/26/2007 03:56 PM, [EMAIL PROTECTED] wrote:
Author: niq
Date: Mon Nov 26 06:56:12 2007
New Revision: 598299
URL: http://svn.apache.org/viewvc?rev=598299view=rev
Log:
mod_filter: don't segfault on (unsupported) chained FilterProviders.
PR 43956
Modified:
httpd/httpd/trunk/CHANGES
httpd/httpd/trunk/modules/filters/mod_filter.c
Modified: httpd/httpd/trunk/CHANGES
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?rev=598299r1=598298r2=598299view=diff
==
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Mon Nov 26 06:56:12 2007
@@ -2,6 +2,9 @@
Changes with Apache 2.3.0
[ When backported to 2.2.x, remove entry from this file ]
+ *) mod_filter: Don't segfault on (unsupported) chained FilterProvider
usage.
+ PR 43956 [Nick Kew]
+
*) mod_unique_id: Fix timestamp value in UNIQUE_ID.
PR 37064 [Kobayashi kobayashi firstserver.co.jp]
Modified: httpd/httpd/trunk/modules/filters/mod_filter.c
URL:
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/filters/mod_filter.c?rev=598299r1=598298r2=598299view=diff
==
--- httpd/httpd/trunk/modules/filters/mod_filter.c (original)
+++ httpd/httpd/trunk/modules/filters/mod_filter.c Mon Nov 26 06:56:12 2007
@@ -137,7 +137,12 @@
harness_ctx *fctx = apr_pcalloc(f-r-pool, sizeof(harness_ctx));
for (p = filter-providers; p; p = p-next) {
-if (p-frec-filter_init_func) {
+if (p-frec-filter_init_func == filter_init) {
+ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f-c,
+ Chaining of FilterProviders not supported);
+return HTTP_INTERNAL_SERVER_ERROR;
+}
+else if (p-frec-filter_init_func) {
f-ctx = NULL;
if ((err = p-frec-filter_init_func(f)) != OK) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, 0, f-c,
Wouldn't it make more sense to prevent this already at configuration time
instead of
failing during request processing (the request processing check seems to be a
good
sanity check that should stay)?
How about deleting the following code from filter_provider?
if (!provider_frec) {
provider_frec = apr_hash_get(cfg-live_filters, pname,
APR_HASH_KEY_STRING);
}
Regards
Rüdiger
Re: [review] upgrade pcre from 6.7 - 7.4 for httpd/trunk
On Mon, 26 Nov 2007, Philip M. Gollucci wrote: I figured after the earlier snafu, I should at least send this to the list for review. I won't do anything with it unless people think its a good thing. Aslo, I mean to add: Affected package: pcre-7.2 Type of problem: pcre -- arbitrary code execution. Reference: http://www.FreeBSD.org/ports/portaudit/bfd6eef4-8c94-11dc-8c55-001c2514716c.html -- Philip M. Gollucci ([EMAIL PROTECTED]) o:703.549.2050x206 Senior System Admin - Riderway, Inc. http://riderway.com / http://ridecharge.com 1024D/EC88A0BF 0DE5 C55C 6BF3 B235 2DAB B89E 1324 9B4F EC88 A0BF Work like you don't need the money, love like you'll never get hurt, and dance like nobody's watching.
Re: Appologies: httpd/httpd/vendor/ SNAFU
On Nov 26, 2007, at 6:59 AM, Philip M. Gollucci wrote: I accidentally committed an upgrade to httpd/httpd/vendor/pcre/current to 7.4. I apparently had a commit bit there because I'm on the PMC from past apreq work. I immediately asked what to do over on #infra on freenode and jerenkrantz agreed I should back it out so I did. Generally speaking, if someone tells you to do something in IRC then it is almost certainly the wrong thing to do -- just like decisions made in boring meetings. The right thing to do, assuming you actually want this change to be done at some point in the near future, is just to apologize for the accident and *ask* if anyone objects to the change *here*. I can't think of any reason not to update the vendor branch with the latest vendor version, particularly after receiving the 37 commit logs associated with it. My opinion won't change, even though the revert means an additional 59 commit notices will be sent for no good reason. The only thing better than that would be to remove the vendor branch entirely, unless the svn log is insufficient to capture what changes have been made since the last time the vendor branch was synced with trunk. In any case, if you have the itch to update trunk to the latest version of PCRE in workable form, then by all means go for it. That is, assuming you have time to test it with httpd first and make sure that it works on your system before committing. Roy
Re: Appologies: httpd/httpd/vendor/ SNAFU
On Nov 26, 2007 4:28 PM, Roy T. Fielding [EMAIL PROTECTED] wrote: Generally speaking, if someone tells you to do something in IRC then it is almost certainly the wrong thing to do -- just like decisions made in boring meetings. Philip said he never intended to commit it. The right thing to do, assuming you actually want this change to be done at some point in the near future, is just to apologize for the accident and *ask* if anyone objects to the change *here*. I did indicate sending the email to [EMAIL PROTECTED] was the priority. *shrug* In any case, if you have the itch to update trunk to the latest version of PCRE in workable form, then by all means go for it. That is, assuming you have time to test it with httpd first and make sure that it works on your system before committing. Once we switched our code to supporting external PCREs, in my opinion, we should have just dropped the whole vendor branch concept as it serves no legitimate purpose any more. If the PCRE guys are doing releases now (it seems someone is home now), then we should just get our changes merged upstream and stop having private copies of it. -- justin
Re: Appologies: httpd/httpd/vendor/ SNAFU
Justin Erenkrantz wrote: Once we switched our code to supporting external PCREs, in my opinion, we should have just dropped the whole vendor branch concept as it serves no legitimate purpose any more. If the PCRE guys are doing releases now (it seems someone is home now), then we should just get our changes merged upstream and stop having private copies of it. -- To Trunk (/future release)? ++1
Re: Appologies: httpd/httpd/vendor/ SNAFU
On Nov 26, 2007 8:01 PM, William A. Rowe, Jr. [EMAIL PROTECTED] wrote: Justin Erenkrantz wrote: Once we switched our code to supporting external PCREs, in my opinion, we should have just dropped the whole vendor branch concept as it serves no legitimate purpose any more. If the PCRE guys are doing releases now (it seems someone is home now), then we should just get our changes merged upstream and stop having private copies of it. -- To Trunk (/future release)? ++1 Yup, we should unbundle PCRE for trunk/2.4/3.0/whatever-comes-next. Obviously, we need to keep bundling it for 2.2 and prior; but going forward? Eh. We only had a PCRE in-tree because we were diverging from upstream and no one on the PCRE side was home for years. So, if someone is maintaining PCRE these days, then we don't need to and just get our folks to download and install PCRE separately. -- justin
Re: Appologies: httpd/httpd/vendor/ SNAFU
On Nov 26, 2007, at 8:20 PM, Justin Erenkrantz wrote: On Nov 26, 2007 8:01 PM, William A. Rowe, Jr. [EMAIL PROTECTED] wrote: Justin Erenkrantz wrote: Once we switched our code to supporting external PCREs, in my opinion, we should have just dropped the whole vendor branch concept as it serves no legitimate purpose any more. If the PCRE guys are doing releases now (it seems someone is home now), then we should just get our changes merged upstream and stop having private copies of it. -- To Trunk (/future release)? ++1 Yup, we should unbundle PCRE for trunk/2.4/3.0/whatever-comes-next. Obviously, we need to keep bundling it for 2.2 and prior; but going forward? Eh. We only had a PCRE in-tree because we were diverging from upstream and no one on the PCRE side was home for years. So, if someone is maintaining PCRE these days, then we don't need to and just get our folks to download and install PCRE separately. -- justin Okay with me. All we need now is a volunteer to figure out what (if any) changes are needed to use a separately installed PCRE. Roy
Re: Appologies: httpd/httpd/vendor/ SNAFU
On Nov 26, 2007 8:46 PM, Roy T. Fielding [EMAIL PROTECTED] wrote:
Okay with me. All we need now is a volunteer to figure out what
(if any) changes are needed to use a separately installed PCRE.
All hail Guido's time machine than has been hijacked by Joe. =) -- justin
% ./configure --help | grep pcre
--with-pcre=PATHUse external PCRE library
r153400 | jorton | 2005-02-11 06:08:24 -0800 (Fri, 11 Feb 2005) | 12 lines
Support use of an external copy of the PCRE library:
* configure.in: Set abs_{builddir,srcdir} higher. Add --with-pcre
flag; build against external PCRE library if used.
* Makefile.in (install-include): Don't install pcre headers any more.
* srclib/Makefile.in (SUBDIRS): Remove.
PR: 27550 (part two)
Submitted by: Andres Salomon dilinger voxel.net, Joe Orton
