[squid-dev] [PATCH] Fetch missing certificates

[Christos Tsantilas]

On 07/11/2016 10:13 PM, Alex Rousskov wrote:
> On 07/11/2016 10:18 AM, Christos Tsantilas wrote:
>
>> +    /// The maximum allowed object size.
>> +    static const size_t MaxObjectSize = 1*1024*1024;
>
>
>> +    bool existingContent = reply ? reply->content_length : 0;
>> +    bool exceedSize = (existingContent > -1 && (size_t)existingContent > MaxObjectSize) ||
>> +        ((object.length() + receivedData.length) > MaxObjectSize);
>
> Wrong type for existingContent. Please use appropriate C++ type casts.
>
> Please refactor to also account for SBuf::maxSize, even if the current
> MaxObjectSize is much smaller.

Stupid bug. I am expecting c++ error in such cases but...


>
> If you can make them const, make them const.
>
> Also, please s/exceedSize/tooLarge/ or s/excessiveSize/.

OK on these. I hope they are ok now.


>
>
>> +        status = Http::scInternalServerError;
>> +        callBack();
>
>> +        status = Http::scOkay;
>> +        callBack();
>
>> +        status = Http::scInternalServerError;
>> +        callBack();
>
>> +        status = Http::scInternalServerError;
>> +        callBack();
>
> Please make status a callBack() parameter instead of a Downloader data
> member.

ok.


>
>
>> +    /// Callback data to use with Downloader callbacks.
>> +    class CbDialer {
>> +    public:
>> +        CbDialer(): status(Http::scNone) {}
>> +        virtual ~CbDialer() {}
>> +        SBuf object;
>> +        Http::StatusCode status;
>> +    };
>
> If you can make Downloader::CbDialer print the status parameter and
> perhaps, for successful downloads, the beginning of object parameter
> (via Raw), please do so. See CallDialer::print().

I did not change anything here.
The PeerConnectorCertDownloaderDialer which is Downloader::CbDialer and 
CallDialer kid implements the print method.

If you prefer we can make the Downloader::CbDialer a CallDialer kid and 
implement the print method.

>
>
>> +    debugs(33, 5, HERE);
>
>> +    debugs(33 , 2, HERE);
>
>> +    debugs(33, 6, HERE);
>
> s/HERE/this/

Well, I use your suggestion for destructors and Amos for functions.
For Downloader probably the AsyncJob::id should used, or print nothing, 
the AsyncJob destructor prints a message in destructor.
Anyway.

>
>
>> +    ClientHttpRequest *const http = new ClientHttpRequest(NULL);
>
> s/NULL/nullptr/ in new code

ok

>
>
> The scope of the following few comments overlap. They should be
> considered together.
>
>> +/// Used to hold and pass the required info and buffers to the
>> +/// clientStream callbacks
>> +class DownloaderContext: public RefCountable
>
> Why do we need a dedicated class with complex lifetime controls [instead
> of using Downloader itself] for this? I am not saying this is wrong yet,
> just asking why it is needed...

Two reasons:
  1) The existing related code for client_side and esi uses a context 
class, and I just follow the same scheme.
  2) The clientStream code requires a RefCountable class.
Because Downloader is an AsyncJob cbdata class and used as cbdata class 
I did not feel safe to use it with clientStream code.

The DownloaderContext class is build for clientStream and destroyed by 
clientStream code after finished. I think it is better to keep this class.

>
>
>> +class DownloaderContext: public RefCountable
>> +{
>> +    CBDATA_CLASS(DownloaderContext);
>
> The combination of cbdata and RefCountable can be deadly as both
> interfaces compete for object lifetime control without knowing about
> each other. IIRC, a few of these ticking time bombs remain in Squid code
> today, but here we are adding new code so, unless existing API require
> this, we should avoid this combination.
>
> I assume that ClientStreams require cbdata; is RefCountable required by
> some other existing API?

No requires RefCoutanble. I added cbdata definitions just for memory 
pools and because I was influenced by the similar code in client_side 
and esi code.
There is not any need for cbdata for this class. I replaced with 
MEMPROXY_CLASS

>
>
>> +DownloaderContext::~DownloaderContext()
>> +{
> ...
>> +    if (http)
>> +        finished();
>> +}
>> +
>> +void
>> +DownloaderContext::finished()
>> +{
>> +    delete http;
>> +    http = NULL;
>> +}
>
>> +void
>> +Downloader::downloadFinished()
>> +{
>> +    debugs(33, 7, this);
>> +    context_->finished();
>
> If we do not need to delete "http" right when downloadFinished() is
> called, then please merge finished() into the DownloaderContext

We need to delete http here.
Unfortunately holds (inderectly) depedencies to context_ object.
The ClientHttpRequest destructor will cause the clientStream objects to 
be released, and also release clientStream refcount to context_ object.

(Yes confusing and difficult to follow. Maybe a subject for a "routing 
parent for ConnStateData/Esi/Downloader classes" project?)

> destructor. Otherwise, perhaps add a source comment to document that we
> cannot delay http destruction until refcounting deletes DownloaderContext.

I add a comment which try to describe the problem.

>
>
>> +     // Calling deleteThis method here to finish Downloader
>> +     // may result to squid crash.
>> +     // This method called by handleReply method which maybe called
>> +     // by ClientHttpRequest::doCallouts. The doCallouts after this object
>> +     // deleted, may operate on non valid objects.
>> +     // Schedule a fake call here just to force squid to delete this object.
>> +     CallJobHere(33, 7, CbcPointer<Downloader>(this), Downloader, downloadFinished);
>
> Does a single async call guarantee that doCallouts() will be able to
> handle our destruction? What if doCallouts() schedules an async call as
> well and comes back after our destruction? Is this a problem our patch
> introduces or an existing/old doCallouts() flaw that we simply need to
> work around, just like some other doCallouts() users?

It is not a problem added by this patch.
It is the doCallouts design. For a reason does not affect other 
subsystems. Probably it is hiden because of tricks like this I did.

This asyncCall will delay ClientHttpRequest destruction after the 
doCallouts will be finished. Else the ClientHttpRequest object will be 
destroyed while the doCallouts runs.

If doCallouts schedule a new callback this is normally checks if the 
ClientHttpRequest object is still not NULL.


>
>
>> +    Downloader* downloader;
>
>> +    if (!cbdataReferenceValid(context->downloader))
>> +        return;
>
> The check only makes sense if context->downloader is a CbcPointer or
> equivalent, not a raw pointer. Otherwise, to reliably test downloader
> validity you need the downloader pointer to be valid (i.e., either it
> always works and the check is not needed or it may crash and the check
> is broken).

I can not see the mistake here.
The cotnext->downloader is set as cbdataReference of cbdata. The pointer 
is always valid but the object maybe is not valid.

I replaced with CbcPointer<Downloader>


>
>
>> +    /// Fake call used internally by Downloader.
>> +    void downloadFinished();
>
> This method/call is as real as any other call. We are forced to call it
> asynchronously because of doCallouts() limitations, but it is not fake.
> I suggest documenting it as "delays destruction to protect doCallouts()"
> or something like that.

ok

>
>
>> +     // Schedule a fake call here just to force squid to delete this object.
>
> Similarly, s/a fake/an async/

ok

>
>
>> +    debugs(33, 4, "Received " << receivedData.length <<
>> +           " object data, offset: " << receivedData.offset <<
>> +           " error flag:" << receivedData.flags.error);
>
> Please move this debugging up, before we may exit the method.

ok

>
> Also, should not we check receivedData.flags.error _before_ we interpret
> any receivedData.length and such?

Well. My sense is that for a reason nobody checks for this flag.
I add a check for failed reply, we may add some other related checks 
here in the future.
For now looks enough.

>
>
>> +    bool exceedSize = (existingContent > -1 && (size_t)existingContent > MaxObjectSize) ||
>> +        ((object.length() + receivedData.length) > MaxObjectSize);
>
> ...
>
>> +    if (receivedData.length > 0) {
>> +        object.append(receivedData.data, receivedData.length);
>> +        http->out.size += receivedData.length;
>> +        http->out.offset += receivedData.length;
>> +    }
>
> The combination does not make sense as far as non-positive
> receivedData.length is concerned:
>
> * If receivedData.length cannot be negative, then the if-statement is
> not needed.
> * If receivedData.length can be negative, then it cannot be used in
> exceedSize calculations without protection.

It can be zero.
I replaced "if (recevedData.length >0)"  with a "if (receivedData.length)"


>
>
> Thank you,
>
> Alex.
>
>

-------------- next part --------------
A non-text attachment was scrubbed...
Name: SQUID-112-fetch-certificates-t3.patch
Type: text/x-patch
Size: 74954 bytes
Desc: not available
URL: <http://lists.squid-cache.org/pipermail/squid-dev/attachments/20160713/f611aeda/attachment-0001.bin>