You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pop3-dev@httpd.apache.org by Sung Kim <hu...@cse.ucsc.edu> on 2003/09/27 09:21:53 UTC
[PATCH] Logging, Two dots, MMAP, etc
Dear Httpd-pop3 developers,
Recently, I found several problems in the module and this patch includes:
1. Logging for each POP3 command and return status.
2. Two dots from LIST and TOP commands.
3. Error on apr_mmap_create. apr_mmap_create will return error for APR_BURRERED file.
4. Added file_close and apr_mmap_delete for cleanup.
Have a nice day.
--
Sung Kim <hu...@cse.ucsc.edu>
http://www.cse.ucsc.edu/~hunkim
"Dreams become reality!"
RE: [PATCH] Logging, Two dots, MMAP, etc
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 06:21 PM 10/6/2003, Sung Kim wrote:
>PS - Can I assume that the previous patch for http_pop3 is rejected? :)
Please don't assume that. The pop3 contributors have alot of cycles on other
projects or security aspects of the httpd project and are sometimes overtaxed.
I'm looking forward to looking at your submission when my P1 queue empties
out again, in the meantime, I hope another committer has some cycles to look
and comment/correct/commit your proposal.
Looks like I need to edit my filters to split this list out of my dev@httpd box,
so it's easier to keep track of your efforts and other's comments :-)
Bill
RE: [PATCH] Logging, Two dots, MMAP, etc
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 06:21 PM 10/6/2003, Sung Kim wrote:
>PS - Can I assume that the previous patch for http_pop3 is rejected? :)
Please don't assume that. The pop3 contributors have alot of cycles on other
projects or security aspects of the httpd project and are sometimes overtaxed.
I'm looking forward to looking at your submission when my P1 queue empties
out again, in the meantime, I hope another committer has some cycles to look
and comment/correct/commit your proposal.
Looks like I need to edit my filters to split this list out of my dev@httpd box,
so it's easier to keep track of your efforts and other's comments :-)
Bill
Re: [PATCH] Logging, Two dots, MMAP, etc
Posted by Sung Kim <hu...@cse.ucsc.edu>.
Hello Cliff,
Thanks for the comments. I attached new patch based on your comments.
Also I'm not very sure about extra apr_rflush. But using the extra apr_rflush solves the two dots problem.
For some reasons, ap_rwrite(".\r\n", strlen(".\r\n"), r) produces two dots.
Have a nice day.
-
Sung Kim <hu...@cse.ucsc.edu>
http://www.cse.ucsc.edu/~hunkim
"Dreams become reality!"
On Sat, 27 Sep 2003, Cliff Woolley wrote:
> On Sat, 27 Sep 2003, Sung Kim wrote:
>
> > 1. Logging for each POP3 command and return status.
> > 2. Two dots from LIST and TOP commands.
> > 3. Error on apr_mmap_create. apr_mmap_create will return error for
> > APR_BURRERED file.
> > 4. Added file_close and apr_mmap_delete for cleanup.
>
>
> Hey, thanks for the patch! A few minorcomments inline:
>
> > +++ pop_mbox.c 27 Sep 2003 06:50:40 -0000
> > @@ -65,7 +65,7 @@
> > apr_status_t pop_parse_maildrop(request_rec *r, pop_mbox **mbox)
> > {
> > apr_status_t rv;
> > - char *str = apr_pcalloc(r->pool, POP_STRING_LENGTH);
> > + char str[POP_STRING_LENGTH];
> > apr_off_t off = 0;
> > int state = 0;
> > int messages = 1;
>
> What's the purpose of this change? Are you seeing a memory leak when
> pcalloc is used? We tend to prefer that against stack buffers whenever
> possible.
>
> > @@ -83,7 +83,7 @@
> >
> > ap_run_translate_name(r);
> >
> > - if ((rv = apr_file_open(&ur->fp, r->filename, APR_READ | APR_WRITE | APR_BUFFERED,
> > + if ((rv = apr_file_open(&ur->fp, r->filename, APR_READ | APR_WRITE,
> > APR_OS_DEFAULT, ur->p)) != APR_SUCCESS) {
> > ap_fprintf(r->output_filters, bb,
> > "-ERR, unable to open %s's maildrop.\n", ur->user);
>
> Good catch!
>
>
> > @@ -111,7 +111,7 @@
> >
> > rv = apr_file_gets(str, bytes, ur->fp);
> > if (rv != APR_SUCCESS && rv != APR_EOF) {
> > - return rv;
> > + return rv;
> > }
> >
> > if ((!strncmp(str, "From ", strlen("From "))) || rv == APR_EOF) {
>
> Please try to avoid stylistic changes in functional patches; it makes them
> harder to review. Also please refer to the coding styleguide at
> http://httpd.apache.org/dev/styleguide.html (in particular, don't use
> tabs :-) ).
>
>
> > @@ -129,7 +129,8 @@
> > state = 1;
> > }
> >
> > - if ((state == 1) && !strcmp(str, "\n")) {
> > + /* Str can be '\n' or CRLF */
> > + if ((state == 1) && (!strcmp(str, "\n") || !strcmp(str, CRLF))) {
> > msg->header_end = off;
> > msg->msg_start = off + 1;
> > state = 2;
>
> If str==CRLF, then shouldn't msg_start=off+2 ?
> (And again, watch those tabs.)
>
>
> > @@ -91,6 +91,7 @@
> >
> > static char *compute_md5(request_rec *r, pop_msg *msg)
> > {
> > + apr_status_t av;
> > apr_mmap_t *mm = NULL;
> > apr_finfo_t finfo;
> > pop_user_rec *ur = (pop_user_rec *)ap_get_module_config(r->request_config,
>
> A very minor nit: by convention we usually call our apr_status_t's "rv"
> rather than "av".
>
>
> > apr_md5_init(ur->ctx);
> > - apr_md5_update(ur->ctx, (char*)mm->mm + msg->header_start, msg->msg_end - msg->header_start);
> > + apr_md5_update(ur->ctx, (char*)mm->mm + msg->header_start,
> > + msg->msg_end - msg->header_start);
> > + apr_mmap_delete(mm);
>
> Again the note on mixing stylistic changes and code changes. I agree that
> this is a good stylistic change, but let's clean up the style in a
> separate patch. :)
>
> @@ -148,6 +154,20 @@
> continue;
> }
> res = handle_func->func(r, buffer);
> > +
> > + /* Fill request_rec for access log */
> > + if (!strcmp(command, "pass"))
> > + r->the_request = apr_psprintf(r->pool, "POP3/%s xxxxxx", command);
> > + else
> > + r->the_request = apr_psprintf(r->pool, "POP3/%s %s", command, buffer);
>
> Very minor point: If you already know command=="pass", you might as well
> just say "POP3/pass xxxxx" instead of using %s, command. Also very minor
> point: Please put then and else conditions inside {}'s, even if they are
> just one line (helps make the code future-proof).
>
> > +
> > + ap_rflush(r);
> > ap_rwrite(".\r\n", strlen(".\r\n"), r);
> > - ap_rflush(r);
> > + ap_rflush(r);
> > }
> > else {
> > generate_scan_listing(r, ur->mbox, atoi(num), 1);
>
> I'm very unclear why this additional flush is necessary. I believe you
> that it is, however it would seem to indicate a bug elsewhere in the httpd
> (like in the buffering code that handles ap_r*() calls). I'll have a peek
> at that code and mention the problem on dev@httpd so that we can maybe
> find the real source of the problem instead of just working around it.
>
> Thanks again for your help!
>
> --Cliff
>
RE: [PATCH] Logging, Two dots, MMAP, etc
Posted by Cliff Woolley <jw...@virginia.edu>.
On Mon, 6 Oct 2003, Sung Kim wrote:
> PS - Can I assume that the previous patch for http_pop3 is rejected? :)
No, I just haven't gotten around to committing it yet. Lots to do, little
time to do it in... :)
If somebody else has a minute, feel free to jump in...
--Cliff
RE: [PATCH] Logging, Two dots, MMAP, etc
Posted by Cliff Woolley <jw...@virginia.edu>.
On Mon, 6 Oct 2003, Sung Kim wrote:
> PS - Can I assume that the previous patch for http_pop3 is rejected? :)
No, I just haven't gotten around to committing it yet. Lots to do, little
time to do it in... :)
If somebody else has a minute, feel free to jump in...
--Cliff
RE: [PATCH] Logging, Two dots, MMAP, etc
Posted by Sung Kim <hu...@soe.ucsc.edu>.
Hello pop3 developers,
We were working on mod_smtp that is a simple SMTP protocol handler using
Apache2 server.
We need mod_pop3 to test mod_smtp. Mod_smtp is getting messages and mod_pop3
transfer mail to MUA.
It means we are testing mod_pop3 a lot!
When I try to retrieve a mail using RETR command, I've got this error. It
seems the error is from deep in somewhere protocol.c.
Do you have any idea?
PS - Can I assume that the previous patch for http_pop3 is rejected? :)
- Sung
(gdb) r -X
Starting program: /usr/local/apache2/bin/httpd -X
[New Thread 8192 (LWP 14150)]
Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 8192 (LWP 14150)]
0x40019828 in apr_brigade_write (b=0xa, flush=0x80981b8 <ap_filter_flush>,
ctx=0x816a338, str=0x80a9a1b "+OK ", nbyte=4) at apr_brigade.c:406
406 apr_bucket *e = APR_BRIGADE_LAST(b);
(gdb) where
#0 0x40019828 in apr_brigade_write (b=0xa, flush=0x80981b8
<ap_filter_flush>,
ctx=0x816a338, str=0x80a9a1b "+OK ", nbyte=4) at apr_brigade.c:406
#1 0x0809a387 in buffer_output (r=0x81799a0, str=0x80a9a1b "+OK ", len=4)
at protocol.c:1369
#2 0x0809a463 in ap_rwrite (buf=0x80a9a1b, nbyte=4, r=0x817a360)
at protocol.c:1404
#3 0x08067656 in generate_scan_listing (r=0x81799a0, mbox=0x817a688,
num=55,
with_ok=1) at pop_protocol.c:222
#4 0x08067d1b in ap_handle_retr (r=0x81799a0, buffer=0x817a357 "")
at pop_protocol.c:499
#5 0x080674be in process_pop_connection_internal (r=0x81799a0,
bb=0x8171a18)
at pop_protocol.c:155
#6 0x08066f64 in process_pop_connection (c=0x8171a18) at pop_core.c:148
#7 0x08096106 in ap_run_process_connection (c=0x8169fb0) at connection.c:85
#8 0x0808bec3 in child_main (child_num_arg=135734136) at prefork.c:694
#9 0x0808c06e in make_child (s=0x810f148, slot=0) at prefork.c:734
#10 0x0808c0c7 in startup_children (number_to_start=5) at prefork.c:806
#11 0x0808c7b9 in ap_mpm_run (_pconf=0x808b7ac, plog=0x8109bc8, s=0x810f148)
at prefork.c:1022
#12 0x080914d2 in main (argc=2, argv=0xbffffa14) at main.c:660
#13 0x420158d4 in __libc_start_main () from /lib/i686/libc.so.6
Re: [PATCH] Logging, Two dots, MMAP, etc
Posted by Cliff Woolley <jw...@virginia.edu>.
On Sat, 27 Sep 2003, Sung Kim wrote:
> 1. Logging for each POP3 command and return status.
> 2. Two dots from LIST and TOP commands.
> 3. Error on apr_mmap_create. apr_mmap_create will return error for
> APR_BURRERED file.
> 4. Added file_close and apr_mmap_delete for cleanup.
Hey, thanks for the patch! A few minorcomments inline:
> +++ pop_mbox.c 27 Sep 2003 06:50:40 -0000
> @@ -65,7 +65,7 @@
> apr_status_t pop_parse_maildrop(request_rec *r, pop_mbox **mbox)
> {
> apr_status_t rv;
> - char *str = apr_pcalloc(r->pool, POP_STRING_LENGTH);
> + char str[POP_STRING_LENGTH];
> apr_off_t off = 0;
> int state = 0;
> int messages = 1;
What's the purpose of this change? Are you seeing a memory leak when
pcalloc is used? We tend to prefer that against stack buffers whenever
possible.
> @@ -83,7 +83,7 @@
>
> ap_run_translate_name(r);
>
> - if ((rv = apr_file_open(&ur->fp, r->filename, APR_READ | APR_WRITE | APR_BUFFERED,
> + if ((rv = apr_file_open(&ur->fp, r->filename, APR_READ | APR_WRITE,
> APR_OS_DEFAULT, ur->p)) != APR_SUCCESS) {
> ap_fprintf(r->output_filters, bb,
> "-ERR, unable to open %s's maildrop.\n", ur->user);
Good catch!
> @@ -111,7 +111,7 @@
>
> rv = apr_file_gets(str, bytes, ur->fp);
> if (rv != APR_SUCCESS && rv != APR_EOF) {
> - return rv;
> + return rv;
> }
>
> if ((!strncmp(str, "From ", strlen("From "))) || rv == APR_EOF) {
Please try to avoid stylistic changes in functional patches; it makes them
harder to review. Also please refer to the coding styleguide at
http://httpd.apache.org/dev/styleguide.html (in particular, don't use
tabs :-) ).
> @@ -129,7 +129,8 @@
> state = 1;
> }
>
> - if ((state == 1) && !strcmp(str, "\n")) {
> + /* Str can be '\n' or CRLF */
> + if ((state == 1) && (!strcmp(str, "\n") || !strcmp(str, CRLF))) {
> msg->header_end = off;
> msg->msg_start = off + 1;
> state = 2;
If str==CRLF, then shouldn't msg_start=off+2 ?
(And again, watch those tabs.)
> @@ -91,6 +91,7 @@
>
> static char *compute_md5(request_rec *r, pop_msg *msg)
> {
> + apr_status_t av;
> apr_mmap_t *mm = NULL;
> apr_finfo_t finfo;
> pop_user_rec *ur = (pop_user_rec *)ap_get_module_config(r->request_config,
A very minor nit: by convention we usually call our apr_status_t's "rv"
rather than "av".
> apr_md5_init(ur->ctx);
> - apr_md5_update(ur->ctx, (char*)mm->mm + msg->header_start, msg->msg_end - msg->header_start);
> + apr_md5_update(ur->ctx, (char*)mm->mm + msg->header_start,
> + msg->msg_end - msg->header_start);
> + apr_mmap_delete(mm);
Again the note on mixing stylistic changes and code changes. I agree that
this is a good stylistic change, but let's clean up the style in a
separate patch. :)
@@ -148,6 +154,20 @@
continue;
}
res = handle_func->func(r, buffer);
> +
> + /* Fill request_rec for access log */
> + if (!strcmp(command, "pass"))
> + r->the_request = apr_psprintf(r->pool, "POP3/%s xxxxxx", command);
> + else
> + r->the_request = apr_psprintf(r->pool, "POP3/%s %s", command, buffer);
Very minor point: If you already know command=="pass", you might as well
just say "POP3/pass xxxxx" instead of using %s, command. Also very minor
point: Please put then and else conditions inside {}'s, even if they are
just one line (helps make the code future-proof).
> +
> + ap_rflush(r);
> ap_rwrite(".\r\n", strlen(".\r\n"), r);
> - ap_rflush(r);
> + ap_rflush(r);
> }
> else {
> generate_scan_listing(r, ur->mbox, atoi(num), 1);
I'm very unclear why this additional flush is necessary. I believe you
that it is, however it would seem to indicate a bug elsewhere in the httpd
(like in the buffering code that handles ap_r*() calls). I'll have a peek
at that code and mention the problem on dev@httpd so that we can maybe
find the real source of the problem instead of just working around it.
Thanks again for your help!
--Cliff