httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Paul J. Reder" <rede...@raleigh.ibm.com>
Subject Re: [Patch]: Make cgis work properly under unix.
Date Mon, 06 Dec 1999 21:03:49 GMT
Ryan Bloom wrote:
> 
> > If this patch is to be vetoed, then the same code needs to be
> > removed from the windows version of proc.c which is where this
> > code was copied from.
> 
> Then the windows code is wrong too.  The fact remains, if we are calling
> ap_create_process, then the arguments should be:
> 
> argv[0] == progname
> argv[1] == arg1
> argv[2] == arg2
> ...
> 
> This is the way POSIX expects it, and we shouldn't be re-inventing things
> here.  If the code doesn't currently do this, then we need to fix it
> correctly, not break it even worse.

I wasn't arguing that the patch shouldn't be vetoed. In fact I agree with you.
All I was doing was pointing out that adding program to argv[0] in mod_cgi
will break the current windows code. I was pointing out that fixing this is
more involved than vetoing unix/proc.c and fixing mod_cgi.

> 
> > Second, all of the places where ap_create_process is called must
> > be checked for argv[] passing. This includes mod_cgi, mod_include,
> > and mod_mime_magic (http_log and mod_rewrite don't pass any argv[]s).
> 
> Actaully, since we used to use POSIX functions, there shouldn't be any
> need to change anything in the main tree, we are bringing our code into
> line with POSIX.

I looked though previous versions of the code and it seems that the mod_XXX
modules didn't add program to the list. This was done in the underlying
utility code that was called to spawn the process. Pedantically speaking,
program has never been added as part of the mod_XXX code. :) You could state
equally validly, however, that program was not added by the "system" call
either (since the spawn code that added it was Apache specific and APR is not).
Modules used to assume that ap_bspawn_child would add program for them (actually
they assumed that ap_bspawn_child would create the whole argv[] list).

I would assume that no caller should pass a NULL argv[] value to ap_create_process
then. They should all pass at least program in the list, yes? If that is the case
then http_log and mod_rewrite both also need to be fixed.

> 
> > Third, I didn't add "my_stupid_string" or "$SHELL_PATH -c". Those
> > were both already there, they were only included as part of the unified
> > diff.
> 
> I never said you put them there.  I explicitly stated these were problems
> I had with the code as it stands now.  These were nits I have that I found
> because they were in your patch, I did not blame you for putting them
> there, you simply brought them to my attention.
> 
> In fact, I am the person who put the SHALL_PATH code in, I just didn't
> remember it.  :->

Sorry I got confused. I just assumed you commented because you saw them in the
patch and missed that they hadn't been newly added.

> 
> Leave the nits alone, because they are minor, and I only brought them up
> because they bugged me when I saw them.  I still stand by my -1, because I
> don't want to break this code even worse than it already is.  If you read
> the documentation in the ap_create_process call, I explicitly state that
> the first argument should be the program name.

Again, I wasn't arguing your -1 (-2 or -3... ;) ). I agree with you.

-- 
Paul J. Reder
------------------------------------------------------------
"Remember, Information is not knowledge; Knowledge is not Wisdom;
Wisdom is not truth; Truth is not beauty; Beauty is not love;
Love is not music; Music is the best." -- Frank Zappa

Mime
View raw message