Re: Geek fun in Squid's source code

[Amos Jeffries]

On 29/06/2012 2:09 p.m., Kinkie wrote:


http://en.wikipedia.org/wiki/P_versus_NP_problem



Ah right.

Then there are the lesser grades of magics ...

  src/stmem.cc:for_each (getNodes().begin(), getNodes().end(), foo);

Amos

On Jun 29, 2012 2:53 AM, "Amos Jeffries" > wrote:


On 29/06/2012 12:01 a.m., Kinkie wrote:

from support_netbios.cc:

if (p == np) {


(stumbled into this while sweeping the sources changing
postincrement
to preincrement operators).

--
 /kinkie



Why do you bring this up?

Set two pointers to the start of a string, increment one until
some delimiter. If they are both still identical the scan produced
an empty string.
 --> Saves loading a constant into memory for comparison and
de-referencing one or both of the pointers to match it against.


What I do note about this is that the CR LF "skip" loop will make
the above logics break and permit binary usernames of "@foo"
or "@foo" through to the backend. I'm not sure if that is
valid? or if there is a missing (++np) operation to actually drop
those octets from the validation.

Amos

Re: Geek fun in Squid's source code

[Robert Collins]

On Fri, Jun 29, 2012 at 12:53 PM, Amos Jeffries  wrote:
> On 29/06/2012 12:01 a.m., Kinkie wrote:
>>
>> from support_netbios.cc:
>>
>> if (p == np) {
>>
>>
>> (stumbled into this while sweeping the sources changing postincrement
>> to preincrement operators).
>>
>> --
>>     /kinkie
>
>
>
> Why do you bring this up?


http://en.wikipedia.org/wiki/P_versus_NP_problem

Re: Geek fun in Squid's source code

[Amos Jeffries]

On 29/06/2012 12:01 a.m., Kinkie wrote:

from support_netbios.cc:

if (p == np) {


(stumbled into this while sweeping the sources changing postincrement
to preincrement operators).

--
 /kinkie



Why do you bring this up?

Set two pointers to the start of a string, increment one until some 
delimiter. If they are both still identical the scan produced an empty 
string.
 --> Saves loading a constant into memory for comparison and 
de-referencing one or both of the pointers to match it against.



What I do note about this is that the CR LF "skip" loop will make the 
above logics break and permit binary usernames of "@foo" or 
"@foo" through to the backend. I'm not sure if that is valid? or if 
there is a missing (++np) operation to actually drop those octets from 
the validation.


Amos

Re: [PATCH] ACL cleanup in 3.2

[Alex Rousskov]

On 06/28/2012 04:59 AM, Amos Jeffries wrote:

> This is my attempted port of Alexs' ACL cleanups. They require the ACL
> extensions changes (what remained in trunk anyway), so teh differences
> there between trunk and 3.2 have been included in this patch.
> 
> So far they are testing out okay on builds, but there is a bit longer to
> go on that.
> 
> If there are no objections I plan to release 3.2.0.18 and apply this
> change to 3.2 branch afterward.


Hi Amos,

Thank you for working on this! My response is not an objection to
these changes going into v3.2, but based on initial feedback from folks
running the cleaned up ACL code in trunk, there is one red flag that we
may need to discuss.

First some terminology. Any single ACL may "match", "mismatch", or
"fail" (in various ways). That result can be negated using "!acl" syntax
in the config. The negation of "match" is "mismatch". The negation of
"mismatch" is "match". The negation of "fail" depends on the code:

  In old code: a negation of a failed ACL was a match.
  In current code: a negation of a failed ACL is a failure.

This means that given a configuration like this:

  some_option allow !group

the old code "allows" or "applies" some_option if the "group" ACL fails.
 After our changes, that option will never be allowed if
the "group" ACL fails (other bugs notwithstanding).


This change breaks old configurations that relied on negated failing
ACLs to match. For example, some admins assumed that if there is no
authentication information, the negated authentication-related ACL will
match. In some cases, those configurations were just broken because they
did not cover all the cases correctly:

http_access allow !badGuys

but there are apparently correctly-working misconfigurations that my
changes break.

I do not have enough information to post any meaningful statistics, but
I suspect that in many cases the admins were thinking that "missing
credentials" is the only ACL failure that can be negated into a match.
However, I believe the code was negating more than that, and possibly
any failure (e.g., a crashed authentication server) could be negated.


IMO, it is wrong to convert failures into matches by negating them. It
is often not logical ("this is not Bob" and "I have no idea who this is"
are often two very different things!). I also predict that this approach
will hinder ACL evolution because adding more operations and/or
extending/optimizing existing ACLs will be difficult when failures are
not propagated properly.

However, even if we agree that failures should not be converted to
matches by negation, there may be thousands of configurations out there
that use this flawed principle. Yes, all of them can be fixed/corrected,
but are we sure that forcing such corrections on users is the best way
forward?


Thank you,

Alex.
P.S. Please note that this discussion is only relevant in the negation
context (!acl) because behavior is meant to be the same in a positive
context.

Geek fun in Squid's source code

[Kinkie]

from support_netbios.cc:

if (p == np) {


(stumbled into this while sweeping the sources changing postincrement
to preincrement operators).

--
/kinkie

Jenkins build is back to normal : 3.2-matrix » master #243

[noc]

See 

[PATCH] ACL cleanup in 3.2

[Amos Jeffries]

This is my attempted port of Alexs' ACL cleanups. They require the ACL 
extensions changes (what remained in trunk anyway), so teh differences 
there between trunk and 3.2 have been included in this patch.


So far they are testing out okay on builds, but there is a bit longer to 
go on that.


If there are no objections I plan to release 3.2.0.18 and apply this 
change to 3.2 branch afterward.


Amos

=== modified file 'src/ExternalACL.h'
--- src/ExternalACL.h   2009-03-08 19:34:36 +
+++ src/ExternalACL.h   2012-06-28 09:22:30 +
@@ -36,7 +36,8 @@
 
 #include "acl/Checklist.h"
 
-class external_acl;
+/** \todo CLEANUP: kill this typedef. */
+typedef struct _external_acl_data external_acl_data;
 
 class ExternalACLLookup : public ACLChecklist::AsyncState
 {
@@ -45,14 +46,15 @@
 static ExternalACLLookup *Instance();
 virtual void checkForAsync(ACLChecklist *)const;
 
+// If possible, starts an asynchronous lookup of an external ACL.
+// Otherwise, asserts (or bails if background refresh is requested).
+static void Start(ACLChecklist *checklist, external_acl_data *acl, bool 
bg);
+
 private:
 static ExternalACLLookup instance_;
 static void LookupDone(void *data, void *result);
 };
 
-/** \todo CLEANUP: kill this typedef. */
-typedef struct _external_acl_data external_acl_data;
-
 #include "acl/Acl.h"
 
 class ACLExternal : public ACL
@@ -61,7 +63,7 @@
 public:
 MEMPROXY_CLASS(ACLExternal);
 
-static void ExternalAclLookup(ACLChecklist * ch, ACLExternal *, EAH * 
callback, void *callback_data);
+static void ExternalAclLookup(ACLChecklist * ch, ACLExternal *);
 
 
 ACLExternal(char const *);

=== modified file 'src/ExternalACLEntry.cc'
--- src/ExternalACLEntry.cc 2012-02-05 06:09:46 +
+++ src/ExternalACLEntry.cc 2012-06-28 08:54:59 +
@@ -69,7 +69,7 @@
 ExternalACLEntry::ExternalACLEntry()
 {
 lru.next = lru.prev = NULL;
-result = 0;
+result = ACCESS_DENIED;
 date = 0;
 def = NULL;
 }

=== modified file 'src/ExternalACLEntry.h'
--- src/ExternalACLEntry.h  2011-02-11 12:53:06 +
+++ src/ExternalACLEntry.h  2012-06-28 08:54:59 +
@@ -44,7 +44,7 @@
 #ifndef SQUID_EXTERNALACLENTRY_H
 #define SQUID_EXTERNALACLENTRY_H
 
-
+#include "acl/Acl.h"
 #include "cbdata.h"
 
 /**
@@ -58,9 +58,9 @@
 {
 
 public:
-ExternalACLEntryData() : result (-1) {}
+ExternalACLEntryData() : result(ACCESS_DUNNO) {}
 
-int result;
+allow_t result;
 #if USE_AUTH
 // TODO use an AuthUser to hold this info
 String user;
@@ -89,7 +89,7 @@
 
 void update(ExternalACLEntryData const &);
 dlink_node lru;
-int result;
+allow_t result;
 time_t date;
 #if USE_AUTH
 String user;

=== modified file 'src/acl/Acl.cc'
--- src/acl/Acl.cc  2012-06-04 10:23:57 +
+++ src/acl/Acl.cc  2012-06-28 09:22:40 +
@@ -317,16 +317,22 @@
 ACLList::matches (ACLChecklist *checklist) const
 {
 assert (_acl);
+// XXX: AclMatchedName does not contain a matched ACL name when the acl
+// does not match (or contains stale name if no ACLs are checked). In
+// either case, we get misleading debugging and possibly incorrect error
+// messages. Unfortunately, deny_info's "when none http_access
+// lines match" exception essentially requires this mess.
+// TODO: Rework by using an acl-free deny_info for the no-match cases?
 AclMatchedName = _acl->name;
 debugs(28, 3, "ACLList::matches: checking " << (op ? null_string : "!") << 
_acl->name);
 
 if (_acl->checklistMatches(checklist) != op) {
 debugs(28, 4, "ACLList::matches: result is false");
-return checklist->lastACLResult(false);
+return false;
 }
 
 debugs(28, 4, "ACLList::matches: result is true");
-return checklist->lastACLResult(true);
+return true;
 }
 
 

=== modified file 'src/acl/Acl.h'
--- src/acl/Acl.h   2011-07-21 11:09:47 +
+++ src/acl/Acl.h   2012-06-28 09:10:04 +
@@ -39,6 +39,10 @@
 #include "cbdata.h"
 #include "dlink.h"
 
+#if HAVE_OSTREAM
+#include 
+#endif
+
 class ConfigParser;
 class ACLChecklist;
 
@@ -105,12 +109,35 @@
 
 /// \ingroup ACLAPI
 typedef enum {
+// Authorization ACL result states
 ACCESS_DENIED,
 ACCESS_ALLOWED,
 ACCESS_DUNNO,
-ACCESS_REQ_PROXY_AUTH
+
+// Authentication ACL result states
+ACCESS_AUTH_REQUIRED,// Missing Credentials
 } allow_t;
 
+inline std::ostream &
+operator <<(std::ostream &o, const allow_t a)
+{
+switch (a) {
+case ACCESS_DENIED:
+o << "DENIED";
+break;
+case ACCESS_ALLOWED:
+o << "ALLOWED";
+break;
+case ACCESS_DUNNO:
+o << "DUNNO";
+break;
+case ACCESS_AUTH_REQUIRED:
+o << "AUTH_REQUIRED";
+break;
+}
+return o;
+}
+
 /// \ingroup ACLAPI
 class acl_access
 {

=== modified file 'src/acl/Checklist.cc'
--- src/acl/Checklist.cc2012