Re: /bzr/squid3/trunk/ r9386: Bug 2395: FTP auth errors not displayed

> Isn't the real question who called FwdState::complete()? It's only meant
> to be called when all data has been placed into the entry, not before..
>

I started with that idea, but the code proved to be very convoluted.

On an error the FTP engine calls ftpFail(), which syncs down to
FtpSate::failed() and generates a 407 error object in the state member for
holding errors, and closes the server links properly and then calls
serverComplete().
  Once serverComplete is called the FwdState jumble kicks off.

There are two functions there complete() and completed()

 The code in complete() appears to handle two distinct cases, (1) when the
connection is done with and everything is okay. And (2) when an error has
occured as seen by *_INCOMPLETE.

On the failure case (2) is active, it tries to store any partial object
received (the 4xx or 5xx message from server???) then calls completed(),
then repeats the storage for squid outgoing error.

By trying to store the partial object before completed() even if it's
empty we actually cause the zero-length-reply state to be triggered by
store and unset the real error message member variable.

I tried doing the calls to copy correct error page into the store object
before calling serverComplete, but that caused worse side effects (such as
status 200 with error page content, or object-already-stored assertions
depending on where it was saved).

This fix, still leaves the pre-save behavior if any entry data was read,
but skips the possibility of generating a zero-length-reply when another
more meaningful error may be present. The errorpage object is left to
wherever the error-handling logics below completed() actually take place.

Amos

>
>
>
>
> tis 2008-12-02 klockan 00:56 +1300 skrev Amos Jeffries:
>> ------------------------------------------------------------
>> revno: 9386
>> committer: Amos Jeffries <squid3_at_treenet.co.nz>
>> branch nick: trunk
>> timestamp: Tue 2008-12-02 00:56:34 +1300
>> message:
>> Bug 2395: FTP auth errors not displayed
>>
>> I appears to be the StoreEntry reporting an error on zero-length
>> objects. This
>> somehow overrides the FTP reported error and aborts the reply page.
>>
>> Add an extra check to prevent StoreEntry::complete() being called too
>> early on error responses.
>> modified:
>> src/forward.cc
>> src/ftp.cc
>> vanligt textdokument-bilaga (r9386.diff)
>> === modified file 'src/forward.cc'
>> --- a/src/forward.cc 2008-10-16 04:51:12 +0000
>> +++ b/src/forward.cc 2008-12-01 11:56:34 +0000
>> @@ -335,9 +335,12 @@
>>
>> startComplete(servers);
>> } else {
>> - debugs(17, 3, "fwdComplete: not re-forwarding status " <<
>> entry->getReply()->sline.status);
>> - EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
>> - entry->complete();
>> + debugs(17, 3, "fwdComplete: server FD " << server_fd << " not
>> re-forwarding status " << entry->getReply()->sline.status);
>> + if (entry->isEmpty() && !err)
>> + {
>> + EBIT_CLR(entry->flags, ENTRY_FWD_HDR_WAIT);
>> + entry->complete();
>> + }
>>
>> if (server_fd < 0)
>> completed();
>>
>> === modified file 'src/ftp.cc'
>> --- a/src/ftp.cc 2008-09-24 13:21:04 +0000
>> +++ b/src/ftp.cc 2008-12-01 11:56:34 +0000
>> @@ -1991,7 +1991,7 @@
>> ftpReadPass(FtpStateData * ftpState)
>> {
>> int code = ftpState->ctrl.replycode;
>> - debugs(9, 3, HERE);
>> + debugs(9, 3, HERE << "code=" << code);
>>
>> if (code == 230) {
>> ftpSendType(ftpState);
>> @@ -3462,7 +3462,11 @@
>> static void
>> ftpFail(FtpStateData *ftpState)
>> {
>> - debugs(9, 3, HERE);
>> + debugs(9, 6, HERE << "flags(" <<
>> + (ftpState->flags.isdir?"IS_DIR,":"") <<
>> + (ftpState->flags.try_slash_hack?"TRY_SLASH_HACK":"") << "),
>> " <<
>> + "mdtm=" << ftpState->mdtm << ", size=" << ftpState->theSize
>> <<
>> + "slashhack=" << (ftpState->request->urlpath.caseCmp("/%2f",
>> 4)==0? "T":"F") );
>>
>> /* Try the / hack to support "Netscape" FTP URL's for retreiving
>> files */
>> if (!ftpState->flags.isdir && /* Not a directory */
>> @@ -3491,6 +3495,7 @@
>> void
>> FtpStateData::failed(err_type error, int xerrno)
>> {
>> + debugs(9,3,HERE << "entry-null=" << (entry?entry->isEmpty():0) <<
>> ", entry=" << entry);
>> if (entry->isEmpty())
>> failedErrorMessage(error, xerrno);
>>
>>
>
Received on Tue Dec 02 2008 - 00:55:58 MST