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

[Alex Rousskov]

On 10/07/2015 12:16 PM, Vitaly Lavrov wrote:
> Bug 4279: No response from proxy for FTP-download of non-existing file
> 
> There is no code to handle errors ftp-protocol functions ftpFail().
> The patch forms a response to the client an error similar to loginFailed().
> To handle specific errors, you must add code in failedHttpStatus().
> 
> If you add to the template ERR_FTP_NOT_FOUND macro "%g", then the client
> will be seen the original message server (550: Permission denied/File
> not found)
> 
> Patch tested on squid-3.5.9


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!

Somebody should examine your additions as well as the existing two
methods and carefully merge them, keeping Ftp::Relay needs in mind. If
you can help with that, I would recommend the following first steps:

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.

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.


HTH,

Alex.