Re: [squid-dev] [PATCH] squid-3.5: fix for errors when receiving a non-existent file via ftp
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. ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
[squid-dev] [PATCH] squid-3.5: use ACL rep_mime_type in delay_access
This worked in squid-2X, but does not work in versions 3.X.
I offer a patch that allows you to use ACL-type "rep_mime_type" in delay_access.
Patch for squid-3.5.9.
This patch working from 3.4.2+ more than one years.
diff --git a/src/DelayId.cc b/src/DelayId.cc
index e776ed3..3fa7972 100644
--- a/src/DelayId.cc
+++ b/src/DelayId.cc
@@ -63,7 +63,7 @@ DelayId::operator bool() const
/* create a delay Id for a given request */
DelayId
-DelayId::DelayClient(ClientHttpRequest * http)
+DelayId::DelayClient(ClientHttpRequest * http, HttpReply *reply)
{
HttpRequest *r;
unsigned short pool;
@@ -85,6 +85,9 @@ DelayId::DelayClient(ClientHttpRequest * http)
}
ACLFilledChecklist ch(DelayPools::delay_data[pool].access, r, NULL);
+if(reply) {
+ ch.reply = HTTPMSGLOCK(reply);
+}
#if FOLLOW_X_FORWARDED_FOR
if (Config.onoff.delay_pool_uses_indirect_client)
ch.src_addr = r->indirect_client_addr;
diff --git a/src/DelayId.h b/src/DelayId.h
index 4bafb57..568f0a9 100644
--- a/src/DelayId.h
+++ b/src/DelayId.h
@@ -12,6 +12,7 @@
#if USE_DELAY_POOLS
class ClientHttpRequest;
+class HttpReply;
#include "DelayIdComposite.h"
/// \ingroup DelayPoolsAPI
@@ -19,7 +20,7 @@ class DelayId
{
public:
-static DelayId DelayClient (ClientHttpRequest *);
+static DelayId DelayClient (ClientHttpRequest *, HttpReply *reply=NULL);
DelayId ();
DelayId (unsigned short);
~DelayId ();
diff --git a/src/client_side_reply.cc b/src/client_side_reply.cc
index 5263bf2..6059bca 100644
--- a/src/client_side_reply.cc
+++ b/src/client_side_reply.cc
@@ -2167,6 +2167,10 @@ clientReplyContext::sendMoreData (StoreIOBuffer result)
cloneReply();
+#if USE_DELAY_POOLS
+if(sc) sc->setDelayId(DelayId::DelayClient(http,reply));
+#endif
+
/* handle headers */
if (Config.onoff.log_mime_hdrs) {
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
[squid-dev] [PATCH] squid-3.5: fix for errors when receiving a non-existent file via ftp
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
diff --git a/src/clients/FtpGateway.cc b/src/clients/FtpGateway.cc
index 524eebb..9039a39 100644
--- a/src/clients/FtpGateway.cc
+++ b/src/clients/FtpGateway.cc
@@ -2438,7 +2438,10 @@ Ftp::Gateway::hackShortcut(FTPSM * nextState)
static void
ftpFail(Ftp::Gateway *ftpState)
{
-debugs(9, 6, HERE << "flags(" <<
+int code = ftpState->ctrl.replycode;
+
+debugs(9, 6, HERE << "state " << ftpState->state <<
+ " reply code " << code << "flags(" <<
(ftpState->flags.isdir?"IS_DIR,":"") <<
(ftpState->flags.try_slash_hack?"TRY_SLASH_HACK":"") << "), " <<
"mdtm=" << ftpState->mdtm << ", size=" << ftpState->theSize <<
@@ -2463,9 +2466,61 @@ ftpFail(Ftp::Gateway *ftpState)
break;
}
}
+if(code >= 400) {
+ ErrorState *err = NULL;
+ err_type error_code = ERR_NONE;
+ const char *command, *reply;
+
+ Http::StatusCode sc = ftpState->failedHttpStatus(error_code);
+ err = new ErrorState(error_code, sc, ftpState->fwd->request);
+
+ // any other problems are general falures.
+ if (!err) {
+ ftpState->failed(ERR_NONE, 0);
+ return;
+ }
+ debugs(9, 5, HERE << "error code " << error_code <<
+ " http code " << sc);
+ {
+ wordlist *w;
+ for(w = ftpState->ctrl.message; w && w->key; w = w->next) {
+ debugs(9, 5, HERE << "ctrl.msg " << w->key);
+ }
+ }
+
+ command = ftpState->old_request ?
+ ftpState->old_request:
+ ftpState->ctrl.last_command;
+
+ if (command && strncmp(command, "PASS", 4) == 0)
+ command = "PASS ";
+
+ reply = ftpState->old_reply ?
+ ftpState->old_reply :
+ ftpState->ctrl.last_reply;
+
+ if (command)
+ err->ftp.request = xstrdup(command);
+
+ if (reply)
+ err->ftp.reply = xstrdup(reply);
+
+ err->ftp.server_msg = ftpState->ctrl.message;
+ ftpState->ctrl.message = NULL;
+
+ err->detailError(code);
+
+ HttpReply *newrep = err->BuildHttpReply();
+ delete err;
+
+ // add it to the store entry for response
+ ftpState->entry->replaceHttpReply(newrep);
+ ftpSendQuit(ftpState);
+} else {
ftpState->failed(ERR_NONE, 0);
/* failed() closes ctrl.conn and frees this */
+}
}
Http::StatusCode
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
[squid-dev] squid 3.4.14, compile without ESI
with these "minimal build" the compilation is successfull :
--build=x86_64-linux-gnu --prefix=/usr --includedir=${prefix}/include
--mandir=${prefix}/share/man --infodir=${prefix}/share/info
--sysconfdir=/etc
--localstatedir=/var --libexecdir=${prefix}/lib/squid3 --srcdir=.
--datadir=/usr/share/squid3 --sysconfdir=/etc/squid3
--mandir=/usr/share/man
--with-swapdir=/var/spool/squid3 --with-logdir=/var/log/squid3
--with-pidfile=/var/run/squid3.pid
--with-filedescriptors=65536 --with-large-files --with-default-user=proxy
--disable-maintainer-mode --disable-dependency-tracking
--enable-silent-rules --disable-translation --disable-auto-locale
# esx guest VM
--disable-arch-native
--enable-async-io=8 --enable-storeio=ufs
--enable-removal-policies=lru,heap
--enable-delay-pools --enable-kill-parent-hack
--disable-ipv6 --disable-pf-transparent --disable-linux-netfilter
--disable-icmp --disable-internal-dns --disable-snmp --disable-unlinkd
--disable-auth --disable-ident-lookups --disable-cache-digests
--disable-eui --disable-htcp --disable-loadable-modules --disable-wccp
--disable-wccpv2
# debugging
--enable-xmalloc-statistics --enable-stacktraces --disable-inline
--disable-optimizations
many thanks, Massimo
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] %ssl::
If there is not any objection I will apply this patch to trunk. On 09/29/2015 06:11 PM, Tsantilas Christos wrote: A new version of this patch. On 09/24/2015 04:11 PM, Amos Jeffries wrote: On 17/09/2015 8:08 p.m., Tsantilas Christos wrote: Currently Squid with SSL bumping only logs SSL errors that have caused Squid to block traffic. It does not log SSL errors that are mimicked. Logging a list with all encountered (and ignored) errors is interesting for debugging and statistics reasons. The new %ssl:: in cf.data.pre: * Please leave a 1-line whitespace gap between these very long descriptions. Same as you can see above the cert_issuer option description. in src/format/Format.cc: * please shuffle the switch case up above the two "not implemented" existing ones. * Also leave whitespace around the new case code. The existing ones are only squashed together since they both fall through to the same break. * sslErrorName can be a static function local to this .cc - that avoids the need to touch Format.h all of the above fixed in this new patch. Amos ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev ___ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev
Re: [squid-dev] [PATCH] external_acl_type logformat tokens
On 10/03/2015 02:35 AM, Amos Jeffries wrote:
> Update the external_acl_type helper interface to use libformat and thus
> make any logformat token valid in its format parameter field.
>
> As a result much of the logic surrounding format code parsing, display
> and helper query generation has been completely dropped. What remains is
> a basic parse loop handling backward compatibility for the unusual
> %CERT_* token syntax, space delimiter and field default encodings.
>
>
> Extensions to logformat resulting from the merger:
>
> * adds \-escape encoding of output fields
>
> * allows {arg} field to be placed before or after the format code.
>
> * extended to accept the old external_acl_type %macros. But not
> documented, these are deprecated and only for backward compatibility.
>
> * extended to support outputting formats without a format-name prefix.
Please rephrase/clarify the last bullet in the commit message. I do not
know what that last bullet means.
> The major side effect of this change is that these ACLs now require
> AccessLogEntry to be filled out with state data, rather than just the
> ACLChecklist object members.
>
> The requires*() mechanism of ACLChecklist has been extended to catch
> some cases resulting from missing the ALE entirely. But it cannot catch
> the more subtle problem of data members inside the ALE being unset.
Agreed. I expect lots of small but time-wasting and
deployment-preventing bugs because of that change.
Would it be possible to avoid most of those problems by using the
following approach?
1. If both hasAleXXX() and requiresAleXXX() are true in
ACL::matches(), call a new virtual checklist->syncAle() method.
2. Add FilledChecklist::syncAle() method that fills unset ALE data
members with data from the checklist object, where possible. This
can be limited to external_acl_type-relevant fields if needed.
3. If any ALE data member is filled in #2, print a level-1 warning.
Do not print more than a few such warnings per worker lifetime.
The above plan is meant to alert us of the bugs introduced by this
change without actually exposing admins to those bugs (except for the
warning and a small performance penalty).
Please let me know if the above sketch is not detailed enough to follow.
> +virtual bool requiresAleXXX() const {return true;}
I do not think this really warrants an XXX because I see nothing
seriously wrong with this method. Same for hasAleXXX().
> // Why is this a sub-class and not a set of real "private:" fields?
> // TODO: shuffle this to the relevant ICP/HTCP protocol section
> class Private
> {
>
> public:
> -Private() : method_str(NULL) {}
> +Private() : method_str(NULL), lastAclName(NULL), lastAclData(NULL) {}
> +~Private() {
> +safe_free(lastAclName);
> +safe_free(lastAclData);
> +}
>
> const char *method_str;
> +const char *lastAclName; ///< string for external_acl_type %ACL
> format code
> +const char *lastAclData; ///< string for external_acl_type %DATA
> format code
> +
> } _private;
Why exacerbate the existing problem by adding more fields to _private
instead of just adding those fields to the "public:" section? Just give
them names specific to external_acl_type to emphasize that these fields
are not generally available (which is true for many ALE fields). You can
even create a dedicated sub-class for them, like we already do for
protocols!
Eventually, we may decide to support lastAclName for access_log (and
other contexts?) as well.
> +safe_free(lastAclData);
...
> +ch->al->_private.lastAclData = sb.c_str();
The combination looks wrong to me. c_str() does not allocate a new
c-string AFAICT.
Please also check the patch for out-of-scope or needless whitespace changes.
This is not a full/detailed audit, but I do not expect to be able to do
more in the foreseeable future. Consider pinging Christos if you need a
second opinion -- he knows this code well.
Thank you,
Alex.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev
