[squid-dev] [PATCH] Fetch missing certificates

[Alex Rousskov]

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.

If you can make them const, make them const.

Also, please s/exceedSize/tooLarge/ or s/excessiveSize/.


> +        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.


> +    /// 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().


> +    debugs(33, 5, HERE);

> +    debugs(33 , 2, HERE);

> +    debugs(33, 6, HERE);

s/HERE/this/


> +    ClientHttpRequest *const http = new ClientHttpRequest(NULL);

s/NULL/nullptr/ in new code


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...


> +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?


> +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
destructor. Otherwise, perhaps add a source comment to document that we
cannot delay http destruction until refcounting deletes DownloaderContext.


> +     // 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?


> +    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).


> +    /// 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.


> +     // Schedule a fake call here just to force squid to delete this object.

Similarly, s/a fake/an async/


> +    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.

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


> +    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.


Thank you,

Alex.