Re: [squid-dev] [PATCH] snprintf result used without validating its range

[Alex Rousskov]

On 02/09/2016 10:25 AM, Yuriy M. Kaminskiy wrote:
> Index: squid-3.5.13/src/log/File.cc
> ===
> --- squid-3.5.13.orig/src/log/File.cc
> +++ squid-3.5.13/src/log/File.cc
> @@ -104,13 +104,16 @@ logfilePrintf(Logfile * lf, const char *
...
> +if (s < 0) {
> +xstrncpy(buf, "snprintf error in logfilePrintf\n", sizeof(buf));
...


We should not write error messages to access.log. When an overflow
happens: If all logfilePrintf() callers cannot meaningfully handle the
error anyway, then we should just log the error message to cache.log and
return from logfilePrintf(). Otherwise, a more complex solution is needed.


N.B. IMHO, logging truncated lines is a bad idea but that wrong decision
was probably made long time ago, and changing it is both difficult and
probably outside your patch scope.


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] convenience library renaming

[Alex Rousskov]

On 02/09/2016 01:57 AM, Amos Jeffries wrote:
> On 9/02/2016 10:08 a.m., Alex Rousskov wrote:
>> On 02/08/2016 01:41 PM, Amos Jeffries wrote:
>>> I have been trying to automate graphing of the Squid internal
>>> dependencies. One of the major issues that has encountered is that some
>>> of our convenience libraries use the '-' hyphen character which is a
>>> reserved character in DOT graph format.
>>>
>>> To make the scripts much simpler and the visual output reflect exactly
>>> what the library names this patch cleans up the library names to follow
>>> our pre-existing policy, and now also to remove punctuation from the
>>> library names.
>>
>> No objections. The library names look inconsistent to me so if you are
>> changing them, you may also want to switch to a single name format:
>> "libX.la", "libsquidX.la", or "libsquid_X.la".

> Uhm. I did.

You did not switch to a single name format.


> This patch is changing those to X/libX.la wherever possible. But leaving
> X/libXsquid.la as the exception

... which results in an inconsistent look-and-feel. If you were trying
to make things look consistent, why pick the format that requires
exceptions?

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PREVIEW] Fetch missing certificates

[Alex Rousskov]

On 02/09/2016 09:59 AM, Amos Jeffries wrote:

> "Download" activity is a ::Client class type of activity, which is about
> managing the Squid end of a Squid->server connection.

I am afraid you got it backwards: Downloader is a request source (and a
response sink). Downloader does not actually care about the
Squid-to-server connection! It is up to Squid to manage Downloader
request and deliver the response back to Downloader (with or without the
connection to the origin server or peer).


> I am aware there is a need for the store logics to be involved. But
> surely StoreEntry and/or clientStreams does not require the whole
> ::Server 'side' of Squid to be involved.

Besides StoreEntry and/or clientStreams, download requests should go
through the regular doCallout() processing sequence including StoreID,
ICAP, and other complex things. Thus, I am sure Downloader needs to be
in the same group of classes as ConnStateData.


> Please no more ConnStateData derived classes. ::Server is the head of
> that hierarchy and the servers/ classes as the per-protocol children.

As for the "Server" parent specifically, I cannot yet say whether your
request is valid. As I said earlier in another thread, I need more time
to study what that unaudited class is and what it should be. Based on
the current "used to manage connections from clients" description alone,
it is obviously the wrong parent for Downloader, but I do not claim that
the description itself is correct.

FWIW, the introduction of that Server class is the primary reason we
have not posted the long-polished "Fetch missing certificates" patch for
review...


> Looking at the patch I also see you have added a thing called
> BinaryTokenizer, but the Tokenizer we already have operates on binary
> octets. All that seems needed is some new accessor(s) for fetching the
> fixed-width binary fields alongside the current int64() ASCII->integer
> decoder.

* Tokenizer API is about sequences of characters -- it is built around
CharacterSet. When it retrieves a number, it may retrieve "012345"
characters.

* BinaryTokenizer is about sequences of bits. When it retrieves a
number, it may retrieve 64 bits.

While it all boils down to "binary octets" at some level, it would be a
design mistake to mix the two concepts in one class IMO; just like it
would be a mistake to add "some accessors" to HTTP parser to parse FTP
messages, even though both HTTP and FTP deal with the same kind of input
token streams.

It is possible to create a common Tokenizer parent for TextTokenizer and
BinaryTokenizer, but I think we should wait for BinaryTokenizer to
mature first. It is possible that it (or another *Tokenizer class using
it) will start dealing with not-byte-aligned bits. If that happens,
building a common parent for all tokenizers would be awkward and
expensive performance-wise.


Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [RFC] What tests would somehow clear 4.1 logically\scientifically\other as *very* stable for production?

[Alex Rousskov]

On 02/09/2016 08:21 AM, Eliezer Croitoru wrote:

> List of practical tests:
> - Forward proxy for HTTP(static objects with size + without size
> declaration, dynamic content from various normal use cases such as
> social networks, academic sources, search engines)
> - Forward proxy for "fake HTTP" requests( I am looking for such
> applications)
> - Forward proxy for basic CONNECT requests with HTTPS, IRC, SKYPE, MAIL
> and couple other basic DESKTOP applications.
> - proxy basic cache manager http(only) basic functionality(no reconf or
> shutdown etc)
> - Forward proxy with ssl_bump and basic splice all settings
> - Forward proxy with ssl_bump and basic peek and splice all settings
> - Forward proxy with ssl_bump and basic peek and splice most settings
> - Forward proxy with ssl_bump and basic peek and splice with specially
> crafted SSL requests
> - Forward proxy with ssl_bump and basic peek and splice with specific
> applications such SKYPE, DROPBOX


The tests you list above would be much better than nothing so thank you
for doing whatever you can.

To answer your question in the Subject line, no amount of lab testing
would clear v4.x as "very stable for production" -- the goal of such
testing ought to be rather different.


FWIW, I am still trying to find the time to finalize the way-overdue
Proper Squid QA plan for the Foundation Board. I am getting closer
though (and we now got more/better tools to back that plan up).


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] convenience library renaming

[Amos Jeffries]

On 9/02/2016 10:08 a.m., Alex Rousskov wrote:
> On 02/08/2016 01:41 PM, Amos Jeffries wrote:
>> I have been trying to automate graphing of the Squid internal
>> dependencies. One of the major issues that has encountered is that some
>> of our convenience libraries use the '-' hyphen character which is a
>> reserved character in DOT graph format.
>>
>> To make the scripts much simpler and the visual output reflect exactly
>> what the library names this patch cleans up the library names to follow
>> our pre-existing policy, and now also to remove punctuation from the
>> library names.
> 
> No objections. The library names look inconsistent to me so if you are
> changing them, you may also want to switch to a single name format:
> "libX.la", "libsquidX.la", or "libsquid_X.la".

Uhm. I did.

There was previously a mix of libX-squid, libXsquid, libsquidX

This patch is changing those to X/libX.la wherever possible. But leaving
X/libXsquid.la as the exception for the cases where we have a path/name
collision with external libraries in the OS environment. libecap and
libssl being the bad ones.


Applied as trunk rev.14530.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

[squid-dev] [RFC] What tests would somehow clear 4.1 logically\scientifically\other as *very* stable for production?

[Eliezer Croitoru]

In relation to the quoted emails (down) about 4.1 Stability.

I was asked more then once the next question:
"What if the proxy goes down???"

Once it was from an IT manager and couple other times in private emails 
and country\work local discussions.

The issues of concern was touched at the article I wrote:
http://wiki.squid-cache.org/Features/StoreID/CollisionRisks

And I wish to honor this amazing IT manager with a few words of mine and 
yours to give some answers about the facts of life, the open-source 
world and from the other side respect his mental stability\sanity after 
being conned more then once in his career.(despite of some of these 
being obviates to many)


I had the pleasure to know some of the heartless and fearsome IT 
managers, the **Kings** of the IT world!
When you actually see a walking dead men you start to understand how the 
IT King actually revives these bodies more then once every day. Is he 
the best in understanding the technology and code? sometimes yes. When 
you see how the code or the service is not really what important for him 
but the Business and the Company workers, you see how it's only a *life 
supporting* system and not real life. You can see how the decisions 
heartless as they might be because of some bad manners\respect\honesty 
or sometimes the result of greed to money\respect\lust\love\fame are 
there and they still happen.
I try to respect the decisions and to understand that many times when 
even I am asked I would answer "Don't pour for me what you pour for 
yourself!!".


The fact of life is that a proxy and many other pieces of software are 
somehow fragile and in them you can see that "High Force" at play and as 
a human you do not have the option to handle\resolve everything. 
Sometimes it's the Kernel and in others it's the Glibc or just the old 
monkey that runs inside the dynamo\generator and keeps pumping the 
electricity into the "BOX".


Then what comes in handy is the belief that somehow even if you cannot 
grasp all these Kernel, libraries and physics you can rely on the 
software engineer good heart\soul\mind\skills to be able to grasp what 
he can and enough to somehow be the King which can revive the situation 
and somehow understand that things are out of someone hands at the moment.


But!!! This burden can never fall on one King each and every time it 
happens. Every King in the Open-Source Kingdoms has it's own advisers, 
friends, trees, walls, food and world. It happens that Kings are being 
replaced by others more suitable names. One of the big examples is SUN 
and Oracle. Did SUN fell? Did Oracle succeeded? I really don't care... 
The only real thing that matters is that as long as the code exists we 
can sense that some developer wrote it. If a human would think 100k 
years(5 years passed in a flash) to the future I believe that any code 
somehow will leave it's mark but in the future will probably need some 
"decrypting" before being used.
Someone in the IT industry mentioned something like "Don't Open-Source 
such an old and unusable code!" and I will not argue but I do agree that 
in some cases it is better for humanity to bury and forget some pieces 
of unusable codes. In the security area some think it's required to "0" 
trillion times the sector and others will prefer to "1" trillion times 
but I think that in-light to this example we can try to at-least 
minimize the options into manageable pieces compared to the goal!!!


Quoting kinkie and many other world experts "OpenStack is not for the 
faint of heart!" and it's the same for IT managers jobs!


Now for the fun and practical part!
My current offer for testing is splitting the task into some kind of 
binary search. Currently the Build-Farm is making sure that somethings 
are done in a way that the CPU and couple other parts will not explode. 
I cannot say a thing about this side automation since I am a manual gear 
person. The humanity for now is relying on these parts enough both from 
the code-reviewing aspect and the drive-testing results.
I can safely say that to delve into these territories you must be "born" 
there and not delve into them randomly. There are some Mighty Kings 
which have grown in these special lands of Super-Ultra-Mighty Kings, 
Warriors Castles and Damsel in distress.
My recommendation for these who wish to delve into these territories to 
take some time and prepare themselves watching and understanding first 
Inception[http://www.imdb.com/title/tt1375666/] (or if you prefer text 
only then beware of being "Transplanted" any non-clear directions).
Also from my tiny experience with these realms it is important to have 
some form of lifeline or as they say in many places around the globe 
"land-line" which you can use in times of need and no... when you are 
deep deep deep inside, Google cannot solve the issue but can help you to 
find some re-direction or "re-think" but again beware.


After RedHat, Oracle and many others are taking care of some 

[squid-dev] Jenkins build is back to normal : trunk-polygraph #943

[noc]

See 

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Security::SessionPointer and Security::LockingPointer

[Christos Tsantilas]

On 02/09/2016 07:31 PM, Amos Jeffries wrote:

On 10/02/2016 5:53 a.m., Christos Tsantilas wrote:

Hi all,

The short question:
The Security::SessionPointer is a TidyPointer. Is it acceptable to
convert it to a LockingPointer?


Only with a reliable reference counting/locking mechanism.

We get away with it on contexts only because contexts are anchored in
permanent globals (ie PortCfg list, ::Config etc) and the deinit()
function does its reference counting relative to active sessions when we
do a -k reconfigure change.

With session_t we dont have any lifesaving anchor and have to use a
slightly different free()-like call that risks totally releasing memory
too early.

I added the fd_table adjustment recently to get us a some stability
anchor, but its not enough to be fully lock-safe in relation to
outstanding Calls or connection close handlers. We still need the code
to pass around the Pointer& instead of Ptr and avoid having more than
one Pointer for each session_t existing at any time.



My sense is that the GnuTLS does not support locking pointers?
However I have an idea about how we can add locking support for
gnutls_session_t if required.



I have not been able to find any exposed refcount/locks we can use in
GnuTLS, and been procrastinating on contacting Nikos about it. GnuTLS
seems to handle threads and SMP internally well enough. But not async
events like our Calls.

I am *very* interested in that locking idea of yours.


I think it is simple to implement it.

The GnuTls API allow to attach user data to the gnutls_session_t 
objects. Someone can use the gnutls_session_set_ptr 
()/gnutls_session_get_ptr () library funtions for this.
We can attach a counter to gnutls_session_set_ptr which increased in 
Security::LockingPointer::resetAndLock() method and decreased in free 
function passed to Security::LockingPointer.







   The Security::SessionPointer is the old Ssl::SSL_Pointer and used to
be a TidiPointer.


Yes.


However while fixing the memory leak bug reported and analysed under the
"[squid-dev] NotePairs, SSL and Cert Validation memory leaks" mail
thread, I made a patch which converted this pointer to a LockingPointer.

If we can not convert it to locking pointer I need to reimplement the
patch. However using a locking pointer for SSL may help us in many other
cases too.



Yes I noticed that. What I've done as a halfway measure is to make the
API of both the Locking and TidyPointers identical. So as a last resort
we could make the OpenSSL one a LockingPointer and the GnuTLS a
TidyPointer. But that does not solve the leak issue for GnuTLS builds.

Amos


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] Security::SessionPointer and Security::LockingPointer

[Amos Jeffries]

On 10/02/2016 9:04 a.m., Christos Tsantilas wrote:
> On 02/09/2016 07:31 PM, Amos Jeffries wrote:
>> On 10/02/2016 5:53 a.m., Christos Tsantilas wrote:
>>> Hi all,
>>>
>>> The short question:
>>> The Security::SessionPointer is a TidyPointer. Is it acceptable to
>>> convert it to a LockingPointer?
>>

> 
> The GnuTls API allow to attach user data to the gnutls_session_t
> objects. Someone can use the gnutls_session_set_ptr
> ()/gnutls_session_get_ptr () library funtions for this.
> We can attach a counter to gnutls_session_set_ptr which increased in
> Security::LockingPointer::resetAndLock() method and decreased in free
> function passed to Security::LockingPointer.
> 

Yes that should work. So long as the resetAndLock also checked that its
datum was not -1, which the non-session objects will pass it.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev