Re: [PATCH] Dynamic SSL Certificate Generation

On 12/11/10 03:05, Tsantilas Christos wrote:
> On 11/11/2010 02:26 PM, Amos Jeffries wrote:
>> On 11/11/10 23:38, Tsantilas Christos wrote:
>>> On 11/11/2010 03:11 AM, Amos Jeffries wrote:
>>>> On Wed, 10 Nov 2010 17:05:16 +0200, Tsantilas Christos
>>>>>
>>>>> * Introduce the TidyPointer class similar to std::auto_ptr, which
>>>>> implements a pointer that deletes the object it points to when the
>>>>> pointer's owner or context is gone. It is designed to avoid memory
>>>>> leaks in the presence of exceptions and processing short cuts.
>>>>
>>>> So whats different about this new pointer type that the existing
>>>> std:auto_ptr, RefCount::Pointer and CbcPointer are all insufficient?
>>>
>>> The TidyPointer is completely different than RefCount and CbcPointers.
>>>
>>> It is similar to the std::auto_ptr but allow developer to define the
>>> deallocator method for the object it points to.
>>> Also does not implement the "=" operator and prevents developers from
>>> using the copy constructor.
>>>
>>> It can be usefull when you have code as the following:
>>> -----------------------
>>> A *a= newA();
>>> if (!a)
>>> return NULL;
>>> B *b=newB();
>>> if (!b) {
>>> freeA(a);
>>> return NULL;
>>> }
>>> C *c=newC();
>>> if (!c) {
>>> freeA(a);
>>> freeB(b);
>>> return NULL;
>>> }
>>>
>>> If(problem){
>>> freeA(a);
>>> freeB(b);
>>> freeC(c);
>>> return NULL;
>>> }
>>> return C;
>>>
>>> Using TidyPointer the above can be written as:
>>>
>>> TidyPointer<A> a(newA(),freeA);
>>> if (!a)
>>> return NULL;
>>> TidyPointer<B> b(newB(),freeB);
>>> if (!b)
>>> return NULL;
>>> TidyPointer<C> c(newC(), freeC);
>>> if (!c)
>>> return NULL;
>>>
>>> if (problem)
>>> return NULL;
>>> return C.release();
>>>
>>>
>>> Can help you to avoid memory leeks when you have the following case:
>>>
>>> A *a= newA();
>>> DoSomeWorkWhichMayThrow(a);
>>> freeA(a);
>>>
>>> In the above case if the DoSomeWorkWhichMayThrow() throws the a object
>>> will never be released.
>>>
>>> The above can be written:
>>>
>>> TidyPointer<A> a(newA(),freeA);
>>> DoSomeWorkWhichMayThrow(a.get());
>>>
>>>>
>>>>> From what I've heard of the SSL problems they are all cause by it not
>>>> using RefCount on the contexts themselves.
>>>>
>>>
>>> Maybe RefCount is usefull for the SSL_CTX objects. But I do not think it
>>> is required.
>>>
>>> In most cases using the openSSL library you have code like the
>>> following:
>>>
>>> a=NewA();
>>> b=newB(a);
>>> if (!b) {
>>> freeA(a);
>>> }
>>> else {
>>> //Just forget the a
>>> }
>>>
>>> I these cases the TidyPointer is very useful.
>>>
>>> Regards,
>>> Christos
>>>
>>
>> Yes. Thank you for that.
>>
>> So it is a special case of RefCount() with enforced maximum count of 1.
> Not exactly.
>
>> Using a specified C-function fro freeing instead of the objects own
>> destructor. So a plus for C objects without destructors.
> Yes.
>
>>
>> Like all the smart pointers TidyPointer will delete it's data if it ever
>> dies. Yet it provides a get() method which allows duplication of the raw
>> pointer. This permits the following case:
>>
>> TidyPointer<a> A(a, free-a);
>> void *B = A->get();
>> A = NULL;
>> if (!B) ...
>
> It does not allow "=" operator to be used as used in your example.
>
> But yes, someone can write:
>
> TidyPointer<a, free-a> A(a);
> void *B = A.get();
> A.reset(NULL);
> if (!B) ...
>
> When the "A.reset(NULL)" statement called the B points to non valid
> memory. But this is a bad use of TidyPointer.
> The correct usage of the above is the following:
>
> TidyPointer<a, free-a> A(a);
> void *B = A.release();
> if (!B) ...
>
> The TidyPointer is designed to be used for local use (eg. inside
> functions/methods ), or passed as const to other functions/methods, or
> for objects allocated once and used as read-only objects until they are
> going to be released.
>
> The TidyPointer does not have implemented "=" operator, does not permit
> copy constructor, it force you to use "::reset(T)" and "::release()"
> methods to modify the pointer. These restrictions are features.
> It is a (new) tool which has completely different purpose of the
> RefCount so I believe should be a different/separate class.
>
>
>>
>> Use of RefCount is preferrable here since B can be recognized.
>>
>>
>> Over in the dynamic generation code this nasty usage case is implemented
>> by the http_port contexts and again in the context "storage".
>>
>> Specifically on the http_port... if you recall the history of bug
>> http://bugs.squid-cache.org/show_bug.cgi?id=2586 the destructor of
>> http_port globals are perfectly capable of cleanly erasing the member
>> contexts. We had to leave the SSL context leaking until they could be
>> *RefCounted* (with a full 1->N count) because active connections held
>> raw-pointers to it long after the "owner" port was closed.
>> Using a TidyPointers there, with get() method in httpsAccept() to attach
>> to SSL* children is exactly identical logic to our previous attempt at
>> making the http_port destructor cleanup.
>
> hmm, I had forgot this bug.
> About SSL_CTX objects (and SSL * ?) we may have to use RefCounts. But it
> is not so simple. You can not just use a different type of pointer.

Yes. So as I see it, until this problems is resolved the http_port will
need to stay using a raw-pointer.
The context "storage" containers will need a look to see if they suffer
the same problem or resolve it somehow.

> The major problem is that we are not passing RefCount or TidyPointers to
> the openSSL library functions. We are passing raw pointers.

If they do not throw that is not a problem. Or if they do throw we much
catch in our caller where the pointer can be treated to special cleanup.

>
> From what I can understand the major problem is that on reconfigure
> there are SSL_CTX objects which are used by some SSL objects so the
> SSL_CTX objects can not be released.

Yes exactly.

>
> Most possible is that we should write a new object classes which
> represents SSL objects and their dependencies on SSL_CTX objects...

Maybe.

Or move to GnuTLS and NSS instead of OpenSSL which has been requested by
several groups of people. Apparently GnuTLS has a nicer API as well,
though I have not looked.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.9
   Beta testers wanted for 3.2.0.3