[Swan-dev] race condition in free_root_certs()

[Andrew Cagney]

On Sat, 1 May 2021 at 15:51, D. Hugh Redelmeier <hugh at mimosa.com> wrote:

> The only place where I notice a refcnt_t manipulated without a lock is
> in free_root_certs.
>
>
Oops, yea.  I don't think it is a problem (but could certainly do with more
notes).  free_root_certs() is called:

- during shutdown and after all the crypto has been cleaned up
-> there should be no helper threads
-> there should be no other references
if this isn't the case then, oops, stumble on - we're going down

- when pluto is idle (as in no request for root certs has happened for 5
minutes)
-> having a structure sit on a reference for 5 minutes is a bug (or I'm
debugging a crypto thread :-)

In the latter case, this kicks in:

        /*
         * Deal with a helper thread being stuck (because it is being
debugged?);
         * need to peek at the refcnt.
         */
        if (root_cert_db->refcnt.count > 1) {
                llog(RC_LOG, logger, "root certs still in use; suspect
stuck thread");
                /* extend or set cert cache lifetime */
                schedule_oneshot_timer(EVENT_FREE_ROOT_CERTS,
FREE_ROOT_CERTS_TIMEOUT);
                return;
        }

Should the race you describe occur, then the next free_root-certs() call in
5 minutes will fix things up.

With that in mind, all I think is needed is a function returning the
refcnt, or even just return that more than one thing is using the object.

(sometimes I wonder if refcnt and the memory allocator should be merged)

One problem is that the refcount can change between being observed and
> the action that is affected by the observation.
>
> Another is that the actual refcnt member isn't guaranteed to be
> atomic.  If all operations were serialized by the mutex, this would
> not be a problem.
>
> The unprotected observation is a test to see if there will be any
> remaining references after this reference is deleted.
>
> It would be simple to change root_certs_delref() to safely return the
> necessary information.  I've actually started to do that.
>
> But it is a little bit awkward that the existing code does not delete
> the reference if the count was larger than one.  My simple change does
> delete the reference.
>
> I guess that the brute-force fix would be to add a function
> "refcnt_delref_if_only".  It doesn't feel optimal.
>
> What's the right fix?
> _______________________________________________
> Swan-dev mailing list
> Swan-dev at lists.libreswan.org
> https://lists.libreswan.org/mailman/listinfo/swan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.libreswan.org/pipermail/swan-dev/attachments/20210501/ed21d162/attachment.html>