Re: The “finalized” SMOB type

[David Kastrup]

Artyom Poptsov <address@hidden> writes:

> Hello Ludovic,
>
> thanks for pointing that out.  IIRC, I saw this kind of errors during
> testing but I thought I fixed them.  What Guile-SSH version do you use?
>
> Here's 'free_session' procedure from Guile-SSH version 0.10.0:
>
> size_t
> free_session (SCM session_smob)
> {
>   if (! SCM_SMOB_PREDICATE (session_tag, session_smob))
>     {
>       _ssh_log (SSH_LOG_FUNCTIONS, "free_session", "%s", "already freed");
>       return 0;
>     }
>   struct session_data *data = _scm_to_session_data (session_smob);
>
>   [...]
>
>   return 0;
> }
>
> As you can see, there's additional smob check before accessing the
> smob's data.

With the current Guile version, you'd always run into the "if" statement
(assuming that free_session is being called via the smob_free hook) and
a session would presumably never be actually freed.

The documentation is rather explicit here:

 -- C Function: void scm_set_smob_free (scm_t_bits tc, size_t (*free)
          (SCM obj))
     This function sets the smob freeing procedure (sometimes referred
     to as a “finalizer”) for the smob type specified by the tag TC.  TC
     is the tag returned by ‘scm_make_smob_type’.

     The FREE procedure must deallocate all resources that are directly
     associated with the smob instance OBJ.  It must assume that all
     ‘SCM’ values that it references have already been freed and are
     thus invalid.

     It must also not call any libguile function or macro except
     ‘scm_gc_free’, ‘SCM_SMOB_FLAGS’, ‘SCM_SMOB_DATA’,
     ‘SCM_SMOB_DATA_2’, and ‘SCM_SMOB_DATA_3’.

     The FREE procedure must return 0.

SCM_SMOB_PREDICATE is explicitly not in the documented allowed
functions, and this has already been the case in Guile-1.8.

> Please let me know if the problem manifest itself in the latest
> Guile-SSH version.

Given the code now used in 2.0.12, I would be quite surprised if this
did not result in a problem of unreleased resources.

The fix from Andy does not cause the behavior to stray outside the
documented bounds, so the question is how much code ignoring those
bounds is actually in the wild and whether the amount is enough to cause
problems.  It's also worth repeating:

     It must assume that all
     ‘SCM’ values that it references have already been freed and are
     thus invalid.

This is not an object in workable state.  Its SCM values, if any, may or
may not have been collected and/or freed already and may be garbage.  So
we are no longer talking about a _valid_ object of the given type.
Should its former type still be _identifiable_ in the standard manner?
I rather suspect that this would actually invite more bugs.

Looking at the actual implementation in commit

I read

+static void*
+clear_smobnum (void *ptr)
+{
+  SCM smob;
+  scm_t_bits smobnum;
+
+  smob = PTR2SCM (ptr);
+
+  smobnum = SCM_SMOBNUM (smob);
+  /* Frob the object's type in place, re-setting it to be the "finalized
+     smob" type.  This will prevent other routines from accessing its
+     internals in a way that assumes that the smob data is valid.  This
+     is notably the case for SMOB's own "mark" procedure, if any; as the
+     finalizer runs without the alloc lock, it's possible for a GC to
+     occur while it's running, in which case the object is alive and yet
+     its data is invalid.  */
+  SCM_SET_SMOB_DATA_0 (smob, SCM_SMOB_DATA_0 (smob) & ~(scm_t_bits) 0xff00);
+
+  return (void *) smobnum;
+}

this looks like only clearing the smob type field, leaving the flags in
place.  So if you really need some former type information for a
decommissioned object, it could be stored in the flags (which are
documented as still being referenceable).

-- 
David Kastrup