Has anyone looked at these bugs yet? I would really appreciate it if someone can comment on the bugs.
On Sat, Aug 22, 2009 at 12:04 PM, Boya Sun <[email protected]> wrote: > Dear Apache-httpd programmers: > > This is Boya Sun from Case Western Reserve University. I have sent you > some potential bugs we discovered in our recent research a few days ago, but > I haven’t got any response yet, so I tried to organize the bugs and > resubmitted these 4 potential bugs. I rewrite the potential bugs and the > potential fixes in the form of patches. Most of the patches are against the > trunk (revision 806655), except for the second one, which is against the > branch of 2.2.x at revision 806782. The patch for BUG2 is not a real bug > fix, but just some comments indicating where the missing code should be > added, since I am not exactly sure how to fix the bug. > > I STRONGLY RECOMMEND you to go over these potential bugs, since these > potential bugs are very similar to some previous bugs in your issue DB or > some revisions that looks like bug-fix, which provide strong evidence that > these potential bugs are real ones. > > In order to make it easier to understand, for each bug we discovered, I > also show the original bug-fix which we used to discover the new bugs. > > I would REALLY appreciate that you could help us confirm whether these > bug-fixes are valid or not, since this is the ONLY way for us to know > whether our approach of discovering new bugs works. > > Thanks very much in advance! And enjoy viewing the bugs……:-) > > BUG1: > Description: This bug was found by analyzing bug 31440 ( > https://issues.apache.org/bugzilla/show_bug.cgi?id=31440); this fix > replaced "srand((int) time((time_t *) NULL))" function with "seed_rand()" in > order to improve rand seed generation under the cases "ALG_APMD5" and > "ALG_CRYPT". > We have found that in the file "htdbm.c", there are code segments that are > very similar to the bug being fixed in 31440. We believe that in this file, > "srand((int) time((time_t *) NULL))" should also be replaced with > "seed_rand()" for cases "ALG_APMD5" and "ALG_CRYPT" > > **********************************original > bug-fix************************************* > --- htpasswd.c (revision 629163) > +++ htpasswd.c (revision 629164) > @@ -126,6 +126,18 @@ > } > } > > +static apr_status_t seed_rand() > +{ > + int seed = 0; > + apr_status_t rv; > + rv = apr_generate_random_bytes((unsigned char*) &seed, sizeof(seed)); > + if (rv) { > + apr_file_printf(errfile, "Unable to generate random bytes: %pm" > NL, rv); > + return rv; > + } > + srand(seed); > + return rv; > +} > > static void putline(apr_file_t *f, const char *l) > { > @@ -174,7 +186,9 @@ > break; > > case ALG_APMD5: > - (void) srand((int) time((time_t *) NULL)); > + if (seed_rand()) { > + break; > + } > generate_salt(&salt[0], 8); > salt[8] = '\0'; > > @@ -190,7 +204,9 @@ > #if (!(defined(WIN32) || defined(TPF) || defined(NETWARE))) > case ALG_CRYPT: > default: > - (void) srand((int) time((time_t *) NULL)); > + if (seed_rand()) { > + break; > + } > to64(&salt[0], rand(), 8); > salt[8] = '\0'; > > *****************************discovered potential bug and possible > fix********************* > --- support/htdbm.c (revision 806655) > +++ support/htdbm.c (working copy) > @@ -298,7 +298,9 @@ > break; > > case ALG_APMD5: > - (void) srand((int) time((time_t *) NULL)); > + if (seed_rand()) { > + break; > + } > to64(&salt[0], rand(), 8); > salt[8] = '\0'; > apr_md5_encode((const char *)htdbm->userpass, (const char > *)salt, > @@ -314,7 +316,9 @@ > break; > #if (!(defined(WIN32) || defined(NETWARE))) > case ALG_CRYPT: > - (void) srand((int) time((time_t *) NULL)); > + if (seed_rand()) { > + break; > + } > to64(&salt[0], rand(), 8); > salt[8] = '\0'; > apr_cpystrn(cpw, crypt(htdbm->userpass, salt), sizeof(cpw) - > 1); > > BUG2: > Description: This bug was found by analyzing revision 602467 ( > http://svn.apache.org/viewvc?view=rev&revision=602467) > The log of this revision is as follows: > > " * core log.c: Work around possible solutions rejected by apr for the old > implementation of apr_proc_create(), and explicitly pass the output and > error channels to all log processes created. This goes all the way back to > piped logs failing to run on win32. Not in or needed at trunk/, as apr 1.3.0 > has the proper fix." > > Note that this bug seems to be particular to the 2.2.x branch, so the ewe > searched for similar bugs in < > https://svn.apache.org/repos/asf/httpd/httpd/branches/2.2.x> at revision > 806782 , instead ot the trunk. > > From the above bug-fix, it seems that before calling "apr_proc_create", > "apr_procattr_child_out_set" and "apr_procattr_child_err_set" should be > invoked on "procattr"; however, we have found a bug that these two functions > are missing where it seems to be appropriate. In this bug, we think that > before calling "ap_os_create_privileged_process", the two functions > "apr_procattr_child_*_set" should be invoked. Note that > "ap_os_create_privileged_process" called "apr_proc_create" inside the > function. > We have also found another evidence that shows that this is likely to be a > real bug, that is, in another file modules/generators/mod_cgid.c, in > function "cgid_server", the two "apr_procattr_child_*_set" functions did > invoked before calling "ap_os_create_privileged_process". > > The patch I attached is not a real bug-fix, but just some comments > indicating where the two missing functions should be added, since I am not > *exactly* sure how to fix the bug. > > **********************************original > bug-fix************************************* > --- log.c (revision 602466) > +++ log.c (revision 602467) > @@ -263,7 +263,7 @@ > apr_status_t rc; > apr_procattr_t *procattr; > apr_proc_t *procnew; > - apr_file_t *errfile; > + apr_file_t *outfile, *errfile; > > if (((rc = apr_procattr_create(&procattr, p)) == APR_SUCCESS) > && ((rc = apr_procattr_cmdtype_set(procattr, > @@ -282,8 +282,11 @@ > pname = apr_pstrdup(p, args[0]); > procnew = (apr_proc_t *)apr_pcalloc(p, sizeof(*procnew)); > > - if (dummy_stderr) { > - if ((rc = apr_file_open_stdout(&errfile, p)) == APR_SUCCESS) > + if ((rc = apr_file_open_stdout(&outfile, p)) == APR_SUCCESS) { > + rc = apr_procattr_child_out_set(procattr, outfile, NULL); > + if (dummy_stderr) > + rc = apr_procattr_child_err_set(procattr, outfile, NULL); > + else if ((rc = apr_file_open_stderr(&errfile, p)) == > APR_SUCCESS) > rc = apr_procattr_child_err_set(procattr, errfile, NULL); > } > > @@ -887,7 +890,13 @@ > else { > char **args; > const char *pname; > + apr_file_t *outfile, *errfile; > > + if ((status = apr_file_open_stdout(&outfile, pl->p)) == > APR_SUCCESS) > + status = apr_procattr_child_out_set(procattr, outfile, NULL); > + if ((status = apr_file_open_stderr(&errfile, pl->p)) == > APR_SUCCESS) > + status = apr_procattr_child_err_set(procattr, errfile, NULL); > + > apr_tokenize_to_argv(pl->program, &args, pl->p); > pname = apr_pstrdup(pl->p, args[0]); > procnew = apr_pcalloc(pl->p, sizeof(apr_proc_t)); > > *****************************discovered potential bug and possible > fix********************* > --- modules/generators/mod_cgi.c （revision 806782） > +++ modules/generators/mod_cgi.c （working copy） > @@ -446,12 +446,16 @@ > ((rc = apr_procattr_addrspace_set(procattr, > e_info->addrspace)) != > APR_SUCCESS) || > ((rc = apr_procattr_child_errfn_set(procattr, cgi_child_errfn)) != > APR_SUCCESS)) { > + /*apr_procattr_child_err_set(procattr,...) and > + apr_procattr_child_out_set(procattr,...) should be invoked here*/ > + > /* Something bad happened, tell the world. */ > ap_log_rerror(APLOG_MARK, APLOG_ERR, rc, r, > "couldn't set child process attributes: %s", > r->filename); > } > else { > procnew = apr_pcalloc(p, sizeof(*procnew)); > + /*ap_proc_create() is invoked inside ap_os_create_privileged_process*/ > rc = ap_os_create_privileged_process(r, procnew, command, argv, > env, > procattr, p); > > BUG3: > Description: This bug was found by analyzing bug 39722 ( > https://issues.apache.org/bugzilla/show_bug.cgi?id=39722); this bug added > a check for the return of ap_server_root_relative; we found a bug where an > additional check for the return value is missing. > > We have observed that almost in all the places where > ap_server_root_relative is invoked, its return value is checked. In the bug > we discovered, however, the return value is not checked. I don’t know > whether it is because that ap_scoreboard_fname is assigned a constant value > DEFAULT_SCOREBOARD. But I think it is safe to add a check to the return > value anyway… > > **********************************original > bug-fix************************************* > --- core.c revision 589176 > +++ core.c revision 589177 > @@ -1164,6 +1164,9 @@ > > /* Make it absolute, relative to ServerRoot */ > arg = ap_server_root_relative(cmd->pool, arg); > + if (arg == NULL) { > + return "DocumentRoot must be a directory"; > + } > > /* TODO: ap_configtestonly && ap_docrootcheck && */ > if (apr_filepath_merge((char**)&conf->ap_document_root, NULL, arg, > > > *****************************discovered potential bug and possible > fix********************* > --- server/scoreboard.c (revision 806655) > +++ server/scoreboard.c (working copy) > @@ -221,7 +221,9 @@ > /* Make sure it's an absolute pathname */ > ap_scoreboard_fname = DEFAULT_SCOREBOARD; > fname = ap_server_root_relative(pconf, ap_scoreboard_fname); > - > + if(!fname){ > + //Add error handling messsage and return > + } > return create_namebased_scoreboard(global_pool, fname); > } > } > > BUG4: > Description: This bug was found by analyzing bug 39518 ( > https://issues.apache.org/bugzilla/show_bug.cgi?id=39518); this bug change > some "apr_palloc / memcpy" construction into a single apr_pmemdup. > > It is actually not a bug-fix, but the change of programming style. > However, I think it’s still worth mentioning some of the code segments we > discovered which need to be refactored the same way as the bug-fix. > > **********************************original > bug-fix************************************* > --- mod_include.c (revision 557836) > +++ mod_include.c (revision 557837) > @@ -3225,9 +3225,8 @@ > > /* check if we mismatched earlier and have to release some > chars */ > if (release && (ctx->flags & SSI_FLAG_PRINTING)) { > - char *to_release = apr_palloc(ctx->pool, release); > + char *to_release = apr_pmemdup(ctx->pool, > intern->start_seq, release); > > - memcpy(to_release, intern->start_seq, release); > newb = apr_bucket_pool_create(to_release, release, > ctx->pool, > f->c->bucket_alloc); > APR_BRIGADE_INSERT_TAIL(pass_bb, newb); > *****************************discovered potential bug and possible > fix********************* > --- server/util_filter.c (revision 806655) > +++ server/util_filter.c (working copy) > @@ -74,9 +74,8 @@ > if (parent->nchildren == parent->size) { > filter_trie_child_ptr *new; > parent->size *= 2; > - new = (filter_trie_child_ptr *)apr_palloc(p, parent->size * > - > sizeof(filter_trie_child_ptr)); > - memcpy(new, parent->children, parent->nchildren * > + > + new = (filter_trie_child_ptr *)apr_pmemdup(p, parent->children, > parent->nchildren * > sizeof(filter_trie_child_ptr)); > parent->children = new; > } > --- modules/http/mod_mime.c (revision 806655) > +++ modules/http/mod_mime.c (working copy) > @@ -182,10 +182,10 @@ > APR_HASH_KEY_STRING); > if (exinfo && *(const char**)((char *)exinfo + suffix[i].offset)) > { > extension_info *copyinfo = exinfo; > - exinfo = (extension_info*)apr_palloc(p, sizeof(*exinfo)); > + exinfo = (extension_info*)apr_pmemdup(p,copyinfo, > sizeof(*exinfo)); > apr_hash_set(mappings, suffix[i].name, > APR_HASH_KEY_STRING, exinfo); > - memcpy(exinfo, copyinfo, sizeof(*exinfo)); > + > *(const char**)((char *)exinfo + suffix[i].offset) = NULL; > } > } > > > > > -- > BOYA SUN > Computer Science Division > Electrical Engineering & Computer Science Department > 513 Olin Building > Case Western Reserve University > 10900 Euclid Avenue > Cleveland, OH 44106 > [email protected] > -- BOYA SUN Computer Science Division Electrical Engineering & Computer Science Department 513 Olin Building Case Western Reserve University 10900 Euclid Avenue Cleveland, OH 44106 [email protected]
