Re: [PATCH] Increase helper-to-Squid buffer size and warn on overflows

mån 2012-02-27 klockan 11:41 -0700 skrev Alex Rousskov:

> Squid's ssl_crtd helper may produce responses exceeding 9907 bytes
> in size (and possibly much larger if multiple chained certificates need
> to be returned to Squid). The old helper.cc code would fill the 8KB read
> buffer completely, schedule a read for zero bytes, receive zero bytes,
> declare an EOF condition, and close the stream (which kills ssl_crtd).
> Due to insufficient information logged, the observable symptoms were
> pretty much the same as if ssl_crtd closed the stream first, indicating
> an ssl_crtd bug.

Ouch!

> BUF_8KB comments indicated that other helpers may use larger-then-8KB
> messages as well although no specific cases were identified.

And url rewriter responses may also be quite large in some cases. And
possibly negotiate auth responses as well.

> We now warn if the read buffer reaches its capacity and kill the
> offending helper explicitly (because we cannot safely recover from this
> condition without implementing growing message accumulation buffers).

But we do have several kinds of growing buffers.. even the raw buffer
type used here supports growing, just not invoked when needed here for
some reason.

Trivial patch attached using the same old raw buffer interface, only
growing the buffer as needed. Please merge with your patch, but keep
default size at 8KB (or less), adjust debug message and level, comments
and commmit the result.

+ if (srv->rbuf_sz - srv->roffset - 1 <= 0)
+ srv->rbuf = (char *)memReallocBuf(srv->rbuf, srv->rbuf_sz +
4096, &srv->rbuf_sz);

> And an increase in minimum buffer capacity to 32KB should make such
> events rare.

Agreed, but not desired. Please keep default size at 8KB or even shrink
it down to 4KB. These are permanent allocations for the duration of the
lifetime of the helper.

> Please note that this minimal patch does not attempt to fix many other
> problems with helper code, including rampant code duplication and fixed
> message buffer size. I am not planning to work on these problems right
> now, and patches to address them are welcomed.

yes this part of the code should be reworked quite a bit next time we
need to touch it, but it works for now.

Regards
Henrik