search

sharplispers / ironclad

A cryptographic toolkit written in Common Lisp
BSD 3-Clause "New" or "Revised" License
166 stars 28 forks source link

Do not close in finalizer on sbcl #29

Closed ralt closed 4 years ago

ralt commented 4 years ago

SBCL itself is already doing the exact same thing, see:

https://github.com/sbcl/sbcl/blob/master/contrib/sb-simple-streams/internal.lisp#L656

which calls: https://github.com/sbcl/sbcl/blob/master/src/code/fd-stream.lisp#L2285-L2292

In my case, I ended up going down that rabbit hole because the FD reported by source (i.e. the underlying fd-stream), aka (sb-sys:fd-stream-fd (slot-value ironclad:*prng* 'ironclad::source)), had a different file descriptor in /proc/$pid/fd/. The behavior I was observing was a stuck read -- most likely because the real file descriptor stored in fd-stream was a socket.

I assume it is related to double-closing, one way or another.

ralt commented 4 years ago

So, the issue with the different file descriptor is actually because it's a dumped image. The finalizer is fine on sbcl, but sbcl is still already doing it. So, do what you want with this PR :)

ralt commented 4 years ago

That said, maybe ironclad could only initialize *prng* on first use to avoid issues with dumped/restored images. (Which was the case before https://github.com/sharplispers/ironclad/commit/36f3685aca747bd61a16b2e051c9fc50c042580d, and it's why it used to work before I upgraded to latest ironclad.)

stassats commented 4 years ago

Or better yet, handle it in sb-ext:*init-hooks*/sb-ext:*save-hooks*

glv2 commented 4 years ago

The finalizer closing '/dev/urandom' was added because of a file descriptor leak with CCL (issue #26). The double close on the stream with SBCL should not be an issue, as according to the hyperspec "it is permissible to close an already closed stream".

I pushed a commit (35cbdcc50da52de5ca8144124cc0471901e13219) to open '/dev/urandom' on first use of an os-prng instead of doing it when creating it. It should fix the problem with the stuck read in dumped images.

A workaround for version 0.49 could be to create a new default prng in the entry point function of a dumped image (with (setf crypto:*prng* (crypto:make-prng :os))).

ralt commented 4 years ago

Thank you! The first use thing is not necessarily perfect though, as a dumped image can still possibly end up with broken FDs, but that probably takes care of the most common issues.