Ensured the TLS context memory is released even if an error occurred.

[cage2]

This patch try to mitigate the problem shown in issue #118

I am still puzzled if :close-callback is useful at all so I am looking forward for reviews and suggestions!

Bye! C.

[avodonosov]

The problem is that the close-callback is not called in case of verification error , as ssl stream is not returned to drakma and drakma is not calling (close ...) on it.

The:auto-free-p t is fine, it will not break normal functioning of the ssl stream. Because, the OpenSSL's CTX objects are reference counted, so the CTX_free means decreasing the reference counter, and if an ssl stream was created successfully, then the CTX will remain alive for internal use, since OpenSSL's SSL object has another reference to it.

But even with this fix the second part of the close-callback will remain not called - the (close s). So open file descriptors can be leaking. Right now I am not sure how to better approach it. Maybe drakma should do something like

(let ((cleanup (lambda () (close s))))
  (handler-bind ((serious-condition (c) (funcall cleanup)))
    (cl+ssl:with-global-context (ctx :auto-free-p t)
        (cl+ssl:make-ssl-client-stream    
          ;; ...
          :close-callback cleanup))))

Or maybe cl+ssl should call the close-callback on error... But that would be somewhat strange, since no-one is calling (close)

[avodonosov]

Thinking more I suppose drakma guarantees the s is closed somewhere upper in the call stack, in case of error.

If so, maybe the :auto-free-p t is enough....

[cage2]

Hi!!

I have checked the opened sockets using ss(8) and i did not found any socket left opened during my test, my best guess is that the socket is closed here:

https://github.com/edicl/drakma/blob/master/request.lisp#L896

Hope this helps! Bye! C.

[avodonosov]

I recomend merging this fix.

It is better to release the ctx reference as soon as drakma code does not need it, in particular in case of error, instead of keeping it around for the close-callback.

[cage2]

Hi @avodonosov !

I changed a bit the patch to ensure the closing callback (and the stream) is closed as soon as possible if an error occurred when starting an ssl-stream.

Do you think this new patch is OK or should i revert to the old one with :auto-free-p t?

Thanks! C.

[avodonosov]

@cage2 , that's for drakma maintainers to decide. IMHO the :auto-free-p t version is better.

[cage2]

Hi!

@cage2 , that's for drakma maintainers to decide. IMHO the :auto-free-p t version is better.

Good point! I have no real strong preference between the two solution, let's wait for their decision then! :)

Bye! C.

[gefjon]

I have a mild preference towards the auto-free-p version as simpler, assuming it does what it says on the tin and fixes this bug. I'd be even happier if it :auto-free-context was added as a keyword argument to make-ssl-stream with a default of t, and a line added to the (currently quite sad in general) docstring describing the behavior of the keyword argument. (An ignorable declaration will also be required if the argument is not used in the non-cl+ssl configurations.) I can't imagine a case where passing :auto-free-context nil to make-ssl-stream will be useful, but the cost of adding a keyword argument is so low that I'd rather cover my ass just in case.

Could you revert to the auto-free-p version, preferably add the auto-free-context arg to make-ssl-stream, and post before-pr and after-pr graphs in this thread like the graph from the issue?

[cage2]

On Mon, Apr 11, 2022 at 10:16:11AM -0700, you wrote:

Hi!

Thanks for considering this patch for merging!

I have a mild preference towards the auto-free-p version as simpler,

Very well!

assuming it does what it says on the tin and fixes this bug.

My only concern is that using auto-free-p the stream is closed elsewhere (in case of an error signalled). But this is a minor (even negligible, i would say) question.

I'd be even happier if it :auto-free-context was added as a keyword argument to make-ssl-stream with a default of t, and a line added to the (currently quite sad in general) docstring describing the behavior of the keyword argument. (An ignorable declaration will also be required if the argument is not used in the non-cl+ssl configurations.) I can't imagine a case where passing :auto-free-context nil to make-ssl-stream will be useful, but the cost of adding a keyword argument is so low that I'd rather cover my ass just in case. Could you revert to the auto-free-p version, preferably add the auto-free-context arg to make-ssl-stream, and post before-pr and after-pr graphs in this thread like the graph from the issue?

Sure, no problem! 👌

Bye! C.

[avodonosov]

@gefjon , adding :auto-free-context to the make-ssl-client-stream is not a good idea.

Let me clarify a little..

I can't imagine a case where passing :auto-free-context nil to make-ssl-stream will be useful

It will be useful most of the time. Many users don't create context and cl+ssl uses a global context instance.

The reason drakma creates context every time is to configure custom "verify locations" - the location of trusted root certificates. Drakma accepts them as parameters of http-request. (BTW, I suppose this leads to reading the certificates files on every every http-request - not perfect for users who use the same values for multiple requests. That's out of scope of the current issue, but can probably be addressed in future drakma changes).

Another reason against the :auto-free-context is that it's more robust to keep the responsibility for context freeing on the party which created the context - the caller of the make-ssl-client-stream.

Why the helper macro cl+ssl:with-global-context provides :auto-free-p is to save the user from creating a local variable:

(cl+ssl:with-global-context
    ((cl+ssl:make-context :verify-depth max-depth
                          ;; ...
                          )
     :auto-free-p t)
  (cl+ssl:make-ssl-client-stream ...))

@cage2 , I'd recommend this for the final patch - since the ctx was only reused on the callback, we can now get rid of it.

That's fine for the helper macro, which will often be used that way. But for make-ssl-client-stream that's not natural.

As a summary - let's go with the :auto-free-p t version.

[gefjon]

adding :auto-free-context to the make-ssl-client-stream is not a good idea. As a summary - let's go with the :auto-free-p t version.

Yeah, that makes sense. Agreed.

[cage2]

Hi!

I reverted the changes until only the first commit is left, i hope I did well! ^^;

Below you can fine the plot for the not patched and patched code, respectively; also for the sake of reproducibility i have added the code i used to collect the data.

Drakma without patch

Drakma patched

The code used for the test

(ql:quickload "drakma")

(defparameter *uri* "https://localhost/")

(defun memory-used ()
  "Assuming only a process whom command line terminate with 'main)'"
  (string-trim '(#\Newline)
               (with-output-to-string (stream)
                 (uiop:run-program "ps aux |  awk -- '/main)$/ {print $6}'"
                                   :output stream))))

(defun main ()
  (loop for i from 0 below 10000 do
    (ignore-errors
      (drakma:http-request *uri*
                          :want-stream nil
                          :verify      :required))
    (sb-ext:gc :full t)
    (format t "~a ~a~%" i (memory-used))))

[gefjon]

Looks good! Thanks for the graphs; that's a really convincing way of demonstrating that your patch fixes the bug.

[cage2]

Thanks to both of you! The help from @avodonosov was essential to understand how to patch the code! C.

[cage2]

Hi!

@cage2 , that's for drakma maintainers to decide. IMHO the :auto-free-p t version is better.

Good point! I have no real strong preference between the two solution, let's wait for their decision then! :)

Bye! C.