Re: [PATCH] Error page format codes upgrade

On 06/10/2013 08:44 PM, Amos Jeffries wrote:
>>> + ErrorState(err_type type, Http::StatusCode, HttpRequest *
>>> request, AccessLogEntry *ale = NULL);

>> I do not see ErrorState constructor being called with an ALE object (the
>> last parameter) anywhere. FwdState, for example, stores ALE but does not
>> pass it to ErrorState. Should not it? Any other contexts where we would
>> be better off passing readily available ALE instead of relying on three
>> levels of volatile and obscure pointers to lead us to it?
>
> The problem is whether ALE is filled out at the time it is passed to
> ErrorState. A lot of the code in server-side is depending on the ALE
> being filled out by logging, long after the error has been generated. I
> would rather use this slightly messy code that fails over to generating
> an ALE filled out with all the required details than suddenly make all
> the errors output by Squid have missing information.

I appreciate the problem. Perhaps add a "Make sure ALE is completely
filled out if you pass it here" comment to the constructor declaration,
to explain why the parameter is currently unused? Or remove the
parameter completely (because nobody uses it).

We may need a function that fills out or _updates_ ALE from currently
available details to solve this problem, but that work is probably
outside your project scope.

>>> + // if request exists, we might have access to the transaction
>>> ALE object
>>> + if (ale_ == NULL && request != NULL &&
>>> request->clientConnectionManager.valid() &&
>>> + request->clientConnectionManager->currentobject != NULL) {
>>> + ClientSocketContext::Pointer c =
>>> request->clientConnectionManager->currentobject;
>>> + ale_ = c->http->al;
>>> + }

>> During pipelining, are we certain that currentobject will point to the
>> request we are creating the error page for??

> No. But that is no more certain when using the accessor in the current
> pipelining design.

My question was about incorrect ClientSocketContext object being used
for error page generation, regardless of the accessor that led to that
object. So, after this change, if pipelining is enabled, then generated
error pages may contain completely bogus information, right? Was that
true before the change? Or is this a problem with the current error page
generation code as well?

If it is an old problem, then you are not responsible for fixing it. If
your code adds it, then you are responsible for avoiding it. That is why
I asked...

>>> /* - IP stuff */
>>> - str.Printf("ClientIP: %s\r\n", src_addr.NtoA(ntoabuf,MAX_IPSTRLEN));
>>> +// char ntoabuf[MAX_IPSTRLEN];
>>> +//XXX str.Printf("ClientIP: %s\r\n", src_addr.NtoA(ntoabuf,MAX_IPSTRLEN));
>> Is this change intentional?
>
> Yes with the src_addr removal we have no way to identify the client IP
> in that code anymore.

It is not clear what that XXX implies. Add a comment?

> If we use fatal() here anyone loading a template with future defined
> codes or typos will crash Squid every time that error is supposed to be
> displayed.

Can Squid validate error templates at startup and quit if templates are
invalid? That would avoid runtime crashes without hiding potentially
serious problems from the admin.

> The assumption that they meanst %% is not great, but we do
> have to choose between hiding the unknown token and displaying it
> un-expanded and since libformat tokens are variable in length we don't
> really have the hide option without potentially screwing something else
> up as well. This way at least it will show up during trivial testing by
> the admin.

What trivial test would expose these problems if Squid does not complain
about them? I cannot think of any (not to mention that most admins would
not know they should run those tests, even if they are trivial).

Thank you,

Alex.
Received on Tue Jun 11 2013 - 21:47:03 MDT