Re: [PATCH] Error page format codes upgrade

On 12/06/2013 9:46 a.m., Alex Rousskov wrote:
> 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.

Was just thinking that while I sent this last patch off.

>
>>>> + // 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?

There are a few comments in places using getCurrentContext() to the
effect that it sometimes supplies the wrong context in pipelines.
I'm not quite sure exactly, but IFF it is possible for async operations
about request #1 in a pipeline to complete after a response to that
request has already been sent, it pops the context off teh queue and may
result in a second response (ie error page) being generated with details
pulled from context of request #2 in the pipeline.

> 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...

The code comments are *very* old as in, some existed before the code was
loaded into CVS. So it may or may not be a real edge case any more.
I am inclined to bet on it not being a problem, some of the other code
is already making that bet and not having any issues yet traced to the
pipeline.

>
>>>> /* - 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?

Do you think we need to display the client IP there anymore? I'm not
sure we do.

>
>> 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.

That would be a part of the internal cache feature which is omitted from
this patch.

I don't think it is a serious problem. Just some technical token
lettering is displayed intead of potentially useful info. All templates
should be tested before use anyway to check that the desired output is
displayed at the desired time.

>
>> 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).

The trivial test of pretending to be a user and loading a forbidden URL
or whatever the use-case they are altering the templates for is.

Not that I'm disagreeing with you on the idea. Just that it is a design
decision that toggles between small internal conversion patch (current)
and major new feature addition (TODO).

PS. The one thing I tell translators if they are trying to figure out
how to handle maros in the grammar is that they are usually representing
some noun, or a full sentence of text. So the context where they appear
should be written in a way that is legible even if the token is
completely unknown to the reader. Macros added by us into the templates
have this principle, macros used by others self-written templates IMO we
can expect to at least have been tried loading to see what the display
looks like before going public.

Amos
Received on Wed Jun 12 2013 - 08:13:31 MDT