tomcat-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Filip Hanik - Dev Lists <devli...@hanik.com>
Subject Re: Bug in B2C converter WAS: svn commit: r568307 - /tomcat/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java
Date Fri, 29 Feb 2008 04:06:40 GMT
Remy Maucherat wrote:
> On Wed, 2008-02-27 at 18:25 -0700, Filip Hanik - Dev Lists wrote:
>   
>> Remy Maucherat wrote:
>>     
>>> On Wed, 2008-02-27 at 17:08 -0700, Filip Hanik - Dev Lists wrote:
>>>   
>>>       
>>>> Remy Maucherat wrote:
>>>>     
>>>>         
>>>>> This makes me think there's a straight issue in CharChunk (I suppose
the
>>>>> problem is that realReadChars gets called with len = 0; if it handled
>>>>> that scenario, things should be fine).
>>>>>   
>>>>>       
>>>>>           
>>>> the old char chunk worked just fine, so why not stick with it. the error

>>>> above is a result of B2CConverter and InputBuffer being modified
>>>>     
>>>>         
>>> Ok, but the new code looks a bit nicer, 
>>>       
>> huh? here we go again, first it was "elegant", now "it's nice". how 
>> about "it works" as argument.
>> bottom line is, my patch was very simple, the B2C Converter is using an 
>> intermediary inputstream, but wasn't looking if there was data available 
>> to read, so that
>>
>> take a look at the original patch proposed
>> http://people.apache.org/~fhanik/tomcat/b2c/patch.txt
>>
>> obviously, my patch touches nothing of what you claim to be "my code", 
>> all I did today was, revert the old patch, and apply the simple patch.
>> and it works.
>>
>>     
>>> so I would prefer keeping it if
>>> possible. From what I can see, your code replaces the limit value (the
>>> one which would cause the problem if it was 0) by the amount of bytes
>>> which have been read in the ByteChunk. This would translate to:
>>> Index: java/org/apache/catalina/connector/InputBuffer.java
>>> ===================================================================
>>> ---java/org/apache/catalina/connector/InputBuffer.java	(revision 630535)
>>> +++java/org/apache/catalina/connector/InputBuffer.java	(working copy)
>>> @@ -355,7 +355,7 @@
>>>          }
>>>  
>>>          state = CHAR_STATE;
>>> -        conv.convert(bb, cb, len);
>>> +        conv.convert(bb, cb, bb.getLength());
>>>          bb.setOffset(bb.getEnd());
>>>  
>>>          return cb.getLength();
>>>
>>> The problem is that I sort of like the idea of the limit (it's related
>>> to the space left in the char buffer for adding the parsed chars). So
>>> maybe trying to respect that with a decent fallback could work (didn't
>>> try it):
>>> Index: java/org/apache/catalina/connector/InputBuffer.java
>>> ===================================================================
>>> ---java/org/apache/catalina/connector/InputBuffer.java	(revision 630535)
>>> +++java/org/apache/catalina/connector/InputBuffer.java	(working copy)
>>> @@ -355,7 +355,7 @@
>>>          }
>>>  
>>>          state = CHAR_STATE;
>>> -        conv.convert(bb, cb, len);
>>> +        conv.convert(bb, cb, (len == 0) ? bb.getLength() : len);
>>>          bb.setOffset(bb.getEnd());
>>>  
>>>          return cb.getLength();
>>>   
>>>       
>> so now we're trying to "fix the fix". why is that? I do NOT claim that 
>> "my shit don't stink" but lets face it, the broken patch came with such 
>> claim. and now were arguing that it's "nicer".
>> sorry for being blunt, but lets cut to the chase here, what is it that 
>> you are really trying to say and maybe we can focus the technical 
>> discussion on that, instead of running around in circles.
>>     
>
> I don't see anything wrong with fixing the thing.
>
>   
>> Obviously the whole byte/char-chunk code is very sensitive, since 
>> several components keep their own offset/end/limit flags to the same 
>> byte[], which makes it somewhat of a difficult thing to read into. and 
>> that's why I managed to come up with a patch that didn't upset that eco 
>> system of the old code.
>>     
>
> I agree, but I don't see this is being proposed. It could be too risky
> obviously.
>   
not really, the patch was fairly large, and I was against for the reason 
of regression when the patch becomes a redo/refactor of how the logic 
works. especially when there is a much simpler fix that does the exact 
thing.
http://svn.apache.org/viewvc/tomcat/tc6.0.x/trunk/java/org/apache/tomcat/util/buf/B2CConverter.java?r1=572497&r2=572496&pathrev=572497

and the comment that went along with the checkin
"The patch I just committed is still a bit fragile (since it depends on 
InputBuffer knowing what CharChunk does internally)"

at this point I don't see how this has to be argued again. the series of 
events speak for themselves.

And Remy, I appreciate how you've handled this, you've kept your cool, I 
had a rough day yesterday, and I apologize if I snapped in my email. Not 
my intention.

Filip

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org


Mime
View raw message