You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "William A. Rowe, Jr." <wr...@rowe-clan.net> on 2005/08/07 01:54:45 UTC
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x:
CHANGES STATUS modules/proxy/proxy_http.c
At 05:28 PM 8/6/2005, Joe Orton wrote:
>On Sat, Aug 06, 2005 at 09:29:13PM -0000, William Rowe wrote:
>> Author: wrowe
>> Date: Sat Aug 6 14:29:05 2005
>> New Revision: 230592
>>
>> URL: http://svn.apache.org/viewcvs?rev=230592&view=rev
>> Log:
>>
>> As much as it pains me, seriously, it seems that reviewing the re-backport
>> of this code was too illegible for review, so it seems we will need to
>> re-review a fresh backport from httpd trunk.
>
>That patch went through the normal 2.0.x review process and received
>three +1s and no vetoes. You absolutely cannot come along a few months
>later and say "oh, actually, -1" and rip stuff out that you now decide
>you don't like.
It received 3 +1 votes, a slim review. It was never released,
so it's not in fact 'done'. If unreleased changes are incorrect,
they need to be fixed, or needs to be reverted.
> You missed the chance to veto
How so?
You can't veto a release. You can veto code; certainly if there
is a 'deadline' it doesn't start until we begin talking about
released code, and that isn't the case here.
> -- if you want to change
>the state of the 2.0.x tree now then you need to go through
>the review process like everyone else does.
I'll respectfully disagree, but I have to ask...
Why do you bring this up now when I mentioned that I had vetoed
the change a good three weeks ago, in STATUS, and advised on
list that it would be reverted?
In any case, I am bringing to STATUS a replacement that is more
correct, and not removing any of the credit well earned by Jeff
Trawick for the original backport (it's just not living in CHANGES
at this moment.)
My 2c - the proxy code is NOT subject to enough review since it's
major refactoring from 1.3 to 2.0. I seriously believe that not
enough dev@ contributors either understand or care enough about
the code to consider it even maintained.
Jeff's comment was dead to right when he originally wrote this
backport, that the patches aren't moving cleanly from trunk to
2.0.x. I couldn't follow the original commit at the time, it
took me over 60 hours to completely review the original backport
and reconsider what needed to be done.
On a friendly note, Joe, you expressed a concern about a possible
segfault regression in trunk; I don't see it in either trunk nor
in 2.0.x with all proposed patches applied. If you wouldn't mind
posting your backtrace I'd much appreciate it.
Bill
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x:
CHANGES STATUS modules/proxy/proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 03:34 AM 8/7/2005, r.pluem@t-online.de wrote:
>Sorry for being confused, but I just want to understand the commit policy/process on 2.0.x better.
See http://httpd.apache.org/dev/voting.html for the definitive
answer, and my post to Jeff Trawick shortly. And don't do what
I did, which was try to use guidelines.html for reference :)
Bill
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x: CHANGES
STATUS modules/proxy/proxy_http.c
Posted by r....@t-online.de.
William A. Rowe, Jr. wrote:
> At 05:28 PM 8/6/2005, Joe Orton wrote:
>
>>On Sat, Aug 06, 2005 at 09:29:13PM -0000, William Rowe wrote:
>>
>>>Author: wrowe
>>>Date: Sat Aug 6 14:29:05 2005
>>>New Revision: 230592
>>>
>>>URL: http://svn.apache.org/viewcvs?rev=230592&view=rev
>>>Log:
>>>
>>> As much as it pains me, seriously, it seems that reviewing the re-backport
>>> of this code was too illegible for review, so it seems we will need to
>>> re-review a fresh backport from httpd trunk.
>>
>>That patch went through the normal 2.0.x review process and received
>>three +1s and no vetoes. You absolutely cannot come along a few months
>>later and say "oh, actually, -1" and rip stuff out that you now decide
>>you don't like.
>
>
> It received 3 +1 votes, a slim review. It was never released,
> so it's not in fact 'done'. If unreleased changes are incorrect,
> they need to be fixed, or needs to be reverted.
>
Sorry for being confused, but I just want to understand the commit policy/process on 2.0.x better.
As far as I understood this right now it works basicly this way:
1. A change to 2.0.x is proposed.
2. It gets 3 binding +1 and no binding -1.
3. The change is commited in subversion.
If some person (with or without binding vote) thinks that the change is -1 in its opinion
after 3. has been executed the process starts from the scratch and reverting this change
follows the same process as any change to 2.0.x and you have to go thru 1. - 3. to get this
reverting done on the 2.0.x branch.
Please advice me if I mixed something up. I just want to understand these things.
Regards
Rüdiger
[..cut..]
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x:
CHANGES STATUS modules/proxy/proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 08:34 PM 8/8/2005, William A. Rowe, Jr. wrote:
>I'M FUCKING SICK AND TIRED OF BEING ASKED TO BRING EACH SPECIFIC
>REVIEWIER THEIR OWN SHINEY FUCKING ROCK.
FYI, I am NOT speaking to your comments exclusively, Roy, nor
Jeff's, nor anyone else's. This just one typical reaction
to being asked for different results by different individuals,
with no grand scheme. There is no scheme since nobody actually
groks every line of proxy_httpd.c. It's a scattershot mess.
Oh; sorry, one individual does; that individual left the project.
Today, I really grok why. But I'm a more stubborn SOB, so you
won't get rid of me that easy :)
[note to casual readers; this is an inside joke - you should
not take it, or my prior post, all too seriously. We know one
another and are (mostly) friendly when presented with a round
of brews :-]
[caution to causal readers; if you want to offer patches to this
list, start small ;-]
Yours,
Bill
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x:
CHANGES STATUS modules/proxy/proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
Quoting myself, and asking for -one- review...
httpd-2.0.55 is stillborn. We will not release as-is with my
veto on trawick's backport of the http proxy request processing
changes. Many voiced an opinion to go forward, so there is code
in a seperate branch to bring the code he backported in sync with
RFC 2616 and our current trunk/.
jimj has voiced one of our 3 +1's. Myself included, it needs
one more +1 review, and more +'s than -'s...
At 08:34 PM 8/8/2005, William A. Rowe, Jr. wrote:
>There is no way in hell I'm refactoring now near 90 hours of work
>to make it more digestable. This big steak I just chowed down
>on Sunday, regurgitating into tiny little digestable pieces for
>chicks to lap up and swallow, is absolutely illustrative of what
>the hell the code should look like. You don't like #7, Joe doesn't
>like #3, Jeff doesn't like #13 and Jim doesn't like #10. For that
>matter I can't even stand where it started from or how I got to the
>end. That's fine, take each little digestable bit. If it doesn't
>break the code, choose to not vote or +1 it. I don't care. If it
>breaks the code, give me technical justification and I'll address
>the complaint.
If any pmc member would please review
http://svn.apache.org/viewcvs/httpd/httpd/branches/proxy-reqbody-2.0.x/modules/proxy/proxy_http.c
from r219059-r230744, we can finally close the logjam. AFAIK,
my employer is the only one shipping -moderately- vulnerability
free code, which is a pity since under both hats I always try
to serve the ASF in turn with my employer, and offer no allegiance
to one over the other. And in fact, my employer knows nothing
of what happens in the security reaction team here until some
public announcement is made. Sad, quite sad.
(You might also consider r171205, the vetoed change, in your
overall evaluation).
I understand few have the patience to review the changes, and
clearly no others understand the code in the first place. I'll
propose under separate email to eject proxy from the httpd core
project in 2.2., given that we have insufficient oversight to
support mod_proxy, unless someone speaks pro or con on technical,
rather than silly grammatical, formatting and procedural matters.
If you don't follow my changes, ask questions. Questions are
always good. There are no embarrassing questions, only sometimes
pathetic answers :)
Bill
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x:
CHANGES STATUS modules/proxy/proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 06:58 PM 8/8/2005, Roy T. Fielding wrote:
>Bill, if you spent more time making your changes understandable
>by documenting what they change instead of various random things
>totally unrelated to each patch, then maybe people like me could
>review them.
Without quoting you at length and spending major bandwidth;
generally I agree; however the existing code needed major surgery.
A victim myself, I'm glad my surgeons have left very clean and
minimal scars; looks like a good arm/finger/leg/unmetionables.
I look at code the same way, remember I jumped in around 1.3.9.
Alot of the code by that point was illegible beyond belief. By
the time we were done with arguments, most of my fixes were
ultimately adopted. That represented about 500 hours of grief.
The resulting code was legible. Not clean, but better.
Fixing these bugs in isolation (just eliminate every patch that
said 'whitespace only', 'no effect', etc) propagates the very
same crap I just wasted my time to fix, and leaves nothing
for the dev behind me to grab on to.
If I need to drop in int i = foo; i += offset; memset(i, n); and
the i was never declared, it's possible to do this midstream with
an {} section. Sure, that's a minimal patch. Is it elegant?
Do we need the extra indention (can we even tolerate it with 80
col considerations?) Is it clear to the reader of the resulting
code? I argue that most of our reviewers are looking at code that
isn't working, not devs reading this list.
These are the points we agree and disagree on. Yes, my patch seeks
to make the end-result comparable to httpd/trunk - and you have no
interest in considering that case. You want simple. So just grab
the branch and see the commits which don't say "whitespace only"
or "noop, cleaning up ...". I was especially careful to consider
that both devs who want the straight line, and devs who want to
know where we are, both can accomplish their goals.
There is no way in hell I'm refactoring now near 90 hours of work
to make it more digestable. This big steak I just chowed down
on Sunday, regurgitating into tiny little digestable pieces for
chicks to lap up and swallow, is absolutely illustrative of what
the hell the code should look like. You don't like #7, Joe doesn't
like #3, Jeff doesn't like #13 and Jim doesn't like #10. For that
matter I can't even stand where it started from or how I got to the
end. That's fine, take each little digestable bit. If it doesn't
break the code, choose to not vote or +1 it. I don't care. If it
breaks the code, give me technical justification and I'll address
the complaint.
I'M FUCKING SICK AND TIRED OF BEING ASKED TO BRING EACH SPECIFIC
REVIEWIER THEIR OWN SHINEY FUCKING ROCK. TAKE THIS FUCKING PATCH
AND SHOVE IT WHEREVER THE FUCK YOU WANT. (Oh, but be certain you
have a long flamewar and debate over the exact composition of the
rocks, the color of the rocks, the diversity and exclusivity of
specific rocks over the others, and don't forget to spend the next
20 years debating the specific metallurgy and carbon dating of the
rocks, arguing over the hand of a maker or natural selection.)
Existing code was 'too broke', so it requires a whole lot of
fixes to all the misbehavior,and a whole lot of fixes to make
the resulting code legible.. By existing, I'm not implicating
Jeff's patch; the origin code we both started with, post
refactor from 1.3, was the issue (heck, probably before then.)
I'm shocked it was voted in without any additional cleanup.
Even more scary, my name might even be on that +1 roster.
Addressing your other issue, when I take more than 3 (read: 4)
hits at a section of code; I'm applying layered multiple bug
fixes with different effects, not fixing my own. If I have
to fix my own code, I hit that in the next commit.
Well, at least one can hope that's the way it goes.
Bill
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x: CHANGES STATUS modules/proxy/proxy_http.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
Bill, if you spent more time making your changes understandable
by documenting what they change instead of various random things
totally unrelated to each patch, then maybe people like me could
review them. Also, it would help a great deal if you would make
a complete set of changes locally, compile and test them locally,
and then present them as a set of logical change sets that achieve
a specific purpose. Right now I am not even bothering to review
your commits because they are obviously untested and consist of
patch-upon-patch-upon-patch fixing what can only be described as
sloppy programming.
For the 2.0 branch, the changes should fix a very specific bug.
There shouldn't be any style changes or variable renaming.
I don't care if it makes it easier to compare with trunk -- I
only want to compare it to 2.0.x-1. If it helps, generate the
patch against the last 2.0.x release tag instead of the branch.
If necessary, we should back out all the 2.0.x changes and release
a 2.0.x without them. I do not consider HTTP request smuggling
to be a blocking issue that requires a redesign of the entire
request reading process. If I did, the answer would be to EOL
the 2.0 branch and move all development off of it.
For the 2.1+ branches, it is imperative that commits be complete
change sets that have been tested on your local machine to solve
the problem you are trying to fix. Some people do that by adding
to the test suite during the process of every change. When I see
more than three commits per change set, I know that the commits
did not work and somebody is just wasting the group's time by
making everyone review unfinished changes. I can understand that
in the heat of the moment every once in a long while, but you are
doing it every single time you work on something in httpd. Just
slow down and make sure the changes are comprehensible before
they are committed and others won't be in such a bad mood when
it comes time to vote on them.
....Roy
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x:
CHANGES STATUS modules/proxy/proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 01:15 PM 8/7/2005, Joe Orton wrote:
>On Sat, Aug 06, 2005 at 06:54:45PM -0500, William Rowe wrote:
>
>> Why do you bring this up now when I mentioned that I had vetoed
>> the change a good three weeks ago, in STATUS, and advised on
>> list that it would be reverted?
>
>Because you putting random crap in STATUS is meaningless. The R-T-C
>process under which the 2.0.x tree is maintained is not.
Ahhh, so your crap in STATUS is called "process", while my crap
in STATUS is called "random crap"? If you didn't agree with my
ability to veto this unreleased, already committed patch you
were welcome to add 2c, your choice of denomination, when I had
changed STATUS. And I would have looked around two weeks ago
and seen that a late veto was invalid. And I'm agreeing with you,
after looking at voting.html, which goes back to 1996. I don't
agree with the policy, as this patch hasn't 'left' Apache yet, but
I agree the policy is clear.
So feel free to cut the crap and start talking to the code, your
comments and attitude have been way out of bounds.
Bottom line; trunk/ had diverged too far from 2.0.x/ - comparing
proxy_http to mod_proxy_http was no longer possible, making it
too difficult to see the changes simply. You are asking to play
hand-me-a-rock, so I'm pelting you with 25 of them. But if there
is anything you don't like at this point, I'm so thoroughly
disgusted with the state of proxy, and the fact that the HTTP
request and response vulnerability reports, from very early on,
interested way too few folks of our security@httpd team, that
you are welcome to pick up the resulting boulder and lug it
around yourself, if you prefer I not svn cp the resulting history
back to httpd-2.0 after 3 +1's. Please don't even bother asking
me to bring you any more rocks, this has cost me dear in sleep
and energy that should have been spent elsewhere.
If anyone considers reviewing each of those 25 commits individually
to be sufficient to ensure the new code is proper, I challenge them
to look at the resulting overall code. It's the small incremental
reviews that let the junk which has accumulated keep piling up.
When blindly +1'ing patches, it's good to read more than 3 lines
back and 3 lines forward. This is why I (should have earlier)
vetoed Jeff's patch; what he did was cool, but the propagated
mistakes in CL/TE elections and other issues became a bigger mess
with the addition of the new three-mode body feature.
Anyways, I trust both you and Jeff find the incremental layers
I've committed satisfactory for review; I didn't commit them in
the same order as they occured in 2.1, I committed them in the
most reasonable order for a dedicated reviewer to understand the
entire scope of changes one piece at a time. I tossed in the last
few just so that you could see *exactly* what is now different
between trunk/ and 2.0.x/, and decide for yourself if they should
differ in the manner they do.
Bill
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x: CHANGES STATUS modules/proxy/proxy_http.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
On Aug 8, 2005, at 5:24 PM, William A. Rowe, Jr. wrote:
> At 06:27 PM 8/8/2005, Roy T. Fielding wrote:
>> Any change that hasn't been released can be vetoed if a technical
>> explanation is given.
>
> Roy; I -totally- agree with your position. However, emails going
> back to 1997 of http://httpd.apache.org/dev/voting.html describe
> a very specific process; votes are collected, result is announced,
> extra +1's are welcome, extra -1's are invalid.
I am sure a lot of people have had an opinion on that since the
voting rules were finished in 1995. However, the only opinion that
has ever been agreed to by the group as a whole is that a veto
is a veto and the only change it can't undo is a released change.
In particular, the voting.html file talks about a process in
which we were preparing releases once per week -- the result
of a vote meant a new release is done. It doesn't say that
explicitly because it was assumed at that time, so the part
that you are reading about votes being invalid after the patch
selection process is done should be read as "after the release
is done". There is a reason why the file is called stale.
I am all for cleaning up the documentation. I am not inclined
to change our process.
> The voting.html is called out as a stale document. Unfortunately,
> the newer and maintained http://httpd.apache.org/dev/guidelines.html
> does not issue an opinion on the matter.
>
> It really seems that if this is the new policy it must be set
> to stone. Two questions in guidelines.html ask;
>
> * We should clarify under what conditions a veto can be
> rescinded or overridden.
>
> * Should we set a time limit on vetos of patches? Two weeks?
>
> There ya go; the current guidelines don't spell it out.
No, they spell it out quite fine. The decision on both questions
was "no". I don't know why the questions were left in the file
after it was approved.
....Roy
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x:
CHANGES STATUS modules/proxy/proxy_http.c
Posted by "William A. Rowe, Jr." <wr...@rowe-clan.net>.
At 06:27 PM 8/8/2005, Roy T. Fielding wrote:
>Any change that hasn't been released can be vetoed if a technical
>explanation is given.
Roy; I -totally- agree with your position. However, emails going
back to 1997 of http://httpd.apache.org/dev/voting.html describe
a very specific process; votes are collected, result is announced,
extra +1's are welcome, extra -1's are invalid.
The voting.html is called out as a stale document. Unfortunately,
the newer and maintained http://httpd.apache.org/dev/guidelines.html
does not issue an opinion on the matter.
It really seems that if this is the new policy it must be set
to stone. Two questions in guidelines.html ask;
* We should clarify under what conditions a veto can be
rescinded or overridden.
* Should we set a time limit on vetos of patches? Two weeks?
There ya go; the current guidelines don't spell it out.
We should remove voting.html if it no longer applies. But we
cannot do so before we update guidelines.html. Better yet,
bubble up guidelines.html as ASF-wide practice in some location
such as www.apache.org/dev/guidelines.html for all projects.
>If you don't like that, then release more often. RTC or CTR is
>irrelevant to the basic principle that code changes can be vetoed
>by any of the core developers.
+= 3 :)
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x: CHANGES STATUS modules/proxy/proxy_http.c
Posted by "Roy T. Fielding" <fi...@gbiv.com>.
Any change that hasn't been released can be vetoed if a technical
explanation is given. If you don't have time to fix it immediately,
then the person who committed the change is required to back the
change out to the point where we are back to the prior release code.
If you don't like that, then release more often. RTC or CTR is
irrelevant to the basic principle that code changes can be vetoed
by any of the core developers.
....Roy
On Aug 7, 2005, at 11:15 AM, Joe Orton wrote:
> On Sat, Aug 06, 2005 at 06:54:45PM -0500, William Rowe wrote:
>> At 05:28 PM 8/6/2005, Joe Orton wrote:
>>> That patch went through the normal 2.0.x review process and received
>>> three +1s and no vetoes. You absolutely cannot come along a few
>>> months
>>> later and say "oh, actually, -1" and rip stuff out that you now
>>> decide
>>> you don't like.
>>
>> It received 3 +1 votes, a slim review. It was never released,
>> so it's not in fact 'done'. If unreleased changes are incorrect,
>> they need to be fixed, or needs to be reverted.
>
> If you now think the changes are incorrect then you need to go through
> the review process to correct them. We've done this before.
>
>>> You missed the chance to veto
>>
>> How so?
>>
>> You can't veto a release. You can veto code; certainly if there
>> is a 'deadline' it doesn't start until we begin talking about
>> released code, and that isn't the case here.
>
> No, you can't veto "code". You vote on *changes to the code*. That's
> what we've been doing for the last N years with 2.0. That's how the
> previous state of the 2.0.x branch was obtained. Again, if you think
> that the tree should be reverted to an older state, then you need to go
> through the normal process.
>
>>> -- if you want to change
>>> the state of the 2.0.x tree now then you need to go through
>>> the review process like everyone else does.
>>
>> I'll respectfully disagree, but I have to ask...
>
> You're making a complete mockery of the time and effort expended by
> those who maintain the 2.0.x tree. Please restore the 2.0.x tree to
> the
> state which was attained through the normal voting process by the
> committers, and stop arguing the toss. Then follow the process like
> everyone else does to try and move *forward*, not backward.
>
> I hope I speak for all the committers here. If anyone thinks this
> request is out of line, please speak up.
>
>> Why do you bring this up now when I mentioned that I had vetoed
>> the change a good three weeks ago, in STATUS, and advised on
>> list that it would be reverted?
>
> Because you putting random crap in STATUS is meaningless. The R-T-C
> process under which the 2.0.x tree is maintained is not.
>
> joe
>
>
Re: svn commit: r230592 - in /httpd/httpd/branches/2.0.x: CHANGES STATUS modules/proxy/proxy_http.c
Posted by Joe Orton <jo...@redhat.com>.
On Sat, Aug 06, 2005 at 06:54:45PM -0500, William Rowe wrote:
> At 05:28 PM 8/6/2005, Joe Orton wrote:
> >That patch went through the normal 2.0.x review process and received
> >three +1s and no vetoes. You absolutely cannot come along a few months
> >later and say "oh, actually, -1" and rip stuff out that you now decide
> >you don't like.
>
> It received 3 +1 votes, a slim review. It was never released,
> so it's not in fact 'done'. If unreleased changes are incorrect,
> they need to be fixed, or needs to be reverted.
If you now think the changes are incorrect then you need to go through
the review process to correct them. We've done this before.
> > You missed the chance to veto
>
> How so?
>
> You can't veto a release. You can veto code; certainly if there
> is a 'deadline' it doesn't start until we begin talking about
> released code, and that isn't the case here.
No, you can't veto "code". You vote on *changes to the code*. That's
what we've been doing for the last N years with 2.0. That's how the
previous state of the 2.0.x branch was obtained. Again, if you think
that the tree should be reverted to an older state, then you need to go
through the normal process.
> > -- if you want to change
> >the state of the 2.0.x tree now then you need to go through
> >the review process like everyone else does.
>
> I'll respectfully disagree, but I have to ask...
You're making a complete mockery of the time and effort expended by
those who maintain the 2.0.x tree. Please restore the 2.0.x tree to the
state which was attained through the normal voting process by the
committers, and stop arguing the toss. Then follow the process like
everyone else does to try and move *forward*, not backward.
I hope I speak for all the committers here. If anyone thinks this
request is out of line, please speak up.
> Why do you bring this up now when I mentioned that I had vetoed
> the change a good three weeks ago, in STATUS, and advised on
> list that it would be reverted?
Because you putting random crap in STATUS is meaningless. The R-T-C
process under which the 2.0.x tree is maintained is not.
joe