[squid-dev] [PATCH] VIA creation code duplication

[Alex Rousskov]

On 03/13/2017 08:25 AM, Eduard Bagdasaryan wrote:
> On 14.02.2017 04:22, Amos Jeffries wrote:
>> The problem is with proxy where the admin has configured large headers
>> to be allowed, and receives a Via just under the 6KB liit. Our append
>> pushing it over by even one byte would assert.

Yes, except s/6/64/ and no special configuration is probably needed:
reply_header_max_size is 64KB by default (and so is String::SizeMax_).


> I am attaching a patch using SBuf instead of String for Via
> accumulating, as you previously suggested. I am not
> sure this is a much better solution since we have to do an
> extra copy into SBuf.

Will this new patch assert in String::setBuffer(?) when the final Via
value exceeds String::SizeMax_ and putStr() is called with a 64KB+1 or
longer value string?


>> The older bbuf code
>> cropping at 1KB was nasty but would not crash Squid.

1KB cropping of the additional Via component before strListAdd() is
pretty much irrelevant to String overflows discussed here -- the size of
the added value is usually not the problem (the resulting total size
is). The reason strListAdd() is better than String::append() when
assembling the Via header is because the former throws and the latter
asserts.


> I don't see the code preventing Squid from String overflow
> there. The bbuf you are talking about was appended to via string
> by strListAdd() without any checks...

strListAdd() has Must(str->canGrowBy(itemSize)). This protection is far
from perfect or even good, but better than an assert.

See trunk r14552 for details.


Wondering how much more time will be wasted on what used to be a simple
and useful code de-duplication patch,

Alex.