[squid-dev] [PATCH] squid-3.5: fix for errors when receiving a non-existent file via ftp

[Amos Jeffries]

On 9/10/2015 3:15 a.m., Vitaly Lavrov wrote:
> On 07.10.2015 22:47, Alex Rousskov wrote:
> 
> Hello Alex.
> 
> Thanks for the quick response.
>> Hello Vitaly,
>>
>>      I am glad you were able to fix the problem, and I thank you for
>> sharing your fix.
>>
>> Unfortunately, the patch itself needs significant refactoring because
>> you are [incorrectly] duplicating an already duplicated code. As you
>> probably know, most of the code you added already exists in
>> Ftp::Gateway::loginFailed() and, more importantly, in
>> Ftp::Client::failed(). We do not need a third imperfect copy of that
>> logic!
> Yes, agree!
>> 0. Undo your changes (the code may be reused at step #3).
>>
>> 1. Add an optional ErrorState *err parameter to ftpFail() with a default
>> NULL value. When the parameter is not nil, ftpFail() should use it
>> instead of creating its own ErrorState.
>>
>> 2. Change Ftp::Gateway::loginFailed() to always call ftpFail() after
>> trying to create an ErrorState. Move the bottom of
>> Ftp::Gateway::loginFailed() (i.e., the code that handles non-nil err) to
>> ftpFail(). Test the refactored code for regressions. This change will
>> not fix the bug you are after.
>>
>> 3. Compare the resulting ftpFail() with the code from your patch. Add
>> any missing actions. Test that the resulting code works fine, including
>> handling the bug you are after.
> I added a field "ErrorState *ftperr" in class Ftp::Client. If it is not
> NULL,
> the Ftp::Client::failed() uses it.

This is not what Alex meant by parameter.

Function parameter is passed by the caller directly and only exists
within the specific function call chain where it was passed. It cannot
remain in memory interferring with other state or operations, does not
need any extra constructor/destructor infrastructure code to manage its
existence, and the value of the pointer being NULL/non-NULL can be used
as the boolean of whether to use it or not.

A member like ftperr may be setup by some earlier FTP transaction an
unknown number of steps and time prior to the current error happening.
Then prevents any following errors from using their own ErrorState
object with details about the second error.

For example; if you have some minor error early in the protocol that the
code causes to be ignored (because its minor), then something major like
login could get an irrelevant error page displayed and break the login
process.


> Through "ftperr" also passed
> httpStatus correct and error-code.
> Duplicate code in loginFailed() and ftpFail() removed.
> 
> See the new version of the patch.
> 
> I can not test all variants of handling errors when using the protocol ftp.
> I checked for proper operation without anonymous access, and anonymous
> access
> to the available file, missing file, missing directory in the file path and
> the file inaccessible due to permissions.
> 
>>
>> More work will be needed after the above steps to remove duplication
>> with Ftp::Client::failed(), but these steps would move us a lot closer
>> to that final goal.
>>

This new patch is making things even more complicated than they started
off just by using that new member. Alexs' suggestion was a specific set
of steps that when followed reduces complexity.

Amos