The “finalized” SMOB type

The “finalized” SMOB type

[Ludovic Courtès]

Hello,

Commit 8dff3af087c6eaa83ae0d72aa8b22aef5c65d65d (introduced in 2.0.12)
changes the type of SMOBs to the “finalized” type just before calling
the SMOB’s free function.

This doesn’t play well with free functions that up to now assumed they
would be getting a SMOB of the type they handle.  One of Guile-SSH’s
free functions goes like this:

--8<---------------cut here---------------start------------->8---
size_t
free_session (SCM session_smob)
{
  struct session_data *data = _scm_to_session_data (session_smob);

  [...]

  return 0;
}
--8<---------------cut here---------------end--------------->8---

… where ‘_scm_to_session_data’ raises a wrong-type-arg error if its
argument is not a session SMOB.  With 2.0.12, this exception is
systematically triggered.

I don’t have a good solution to this and I think Guile-SSH will have to
work around it anyway, but I think this situation is not uncommon.

Thoughts?

Ludo’.

Re: The “finalized” SMOB type

[David Kastrup]

ludo@gnu.org (Ludovic Courtès) writes:

> Hello,
>
> Commit 8dff3af087c6eaa83ae0d72aa8b22aef5c65d65d (introduced in 2.0.12)
> changes the type of SMOBs to the “finalized” type just before calling
> the SMOB’s free function.
>
> This doesn’t play well with free functions that up to now assumed they
> would be getting a SMOB of the type they handle.  One of Guile-SSH’s
> free functions goes like this:
>
> size_t
> free_session (SCM session_smob)
> {
>   struct session_data *data = _scm_to_session_data (session_smob);
>
>   [...]
>
>   return 0;
> }
>
> … where ‘_scm_to_session_data’ raises a wrong-type-arg error if its
> argument is not a session SMOB.  With 2.0.12, this exception is
> systematically triggered.
>
> I don’t have a good solution to this and I think Guile-SSH will have to
> work around it anyway, but I think this situation is not uncommon.
>
> Thoughts?

The alternative to using a single "finalized" type would be to sacrifice
a whole bit for the "finalized" information.  This bit would stop the
type from getting sweeped (for example using the mark_smob hook) while
still allowing the type to be retrieved.

However, once the type is stopped from getting sweeped, all of its
internals are up for being collected and finalized, possibly _before_
the type itself.

Now Guile uses Java collection semantics (set somewhere obscure in the
guardian module if memory serves me right) which is described something
like

GC_API GC_ATTR_DEPRECATED int GC_java_finalization;
                        /* Mark objects reachable from finalizable      */
                        /* objects in a separate post-pass.  This makes */
                        /* it a bit safer to use non-topologically-     */
                        /* ordered finalization.  Default value is      */
                        /* determined by JAVA_FINALIZATION macro.       */
                        /* Enables register_finalizer_unreachable to    */
                        /* work correctly.                              */
                        /* The setter and getter are unsynchronized.    */
GC_API void GC_CALL GC_set_java_finalization(int);
GC_API int GC_CALL GC_get_java_finalization(void);

This might be enough to make the scheme work.  A whole bit of type field
might be much, however.  I seem to remember that there are only 8 to go
around anyway.

-- 
David Kastrup

Re: The “finalized” SMOB type

[Artyom Poptsov]

[-- Attachment #1: Type: text/plain, Size: 980 bytes --]

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:

--8<---------------cut here---------------start------------->8---
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;
}
--8<---------------cut here---------------end--------------->8---

As you can see, there's additional smob check before accessing the
smob's data.

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

Thanks!

- Artyom

-- 
Artyom V. Poptsov <poptsov.artyom@gmail.com>;  GPG Key: 0898A02F
Home page: http://poptsov-artyom.narod.ru/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]

Re: The “finalized” SMOB type

[David Kastrup]

Artyom Poptsov <poptsov.artyom@gmail.com> 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

Re: The “finalized” SMOB type

[Ludovic Courtès]

Hi Artyom,

Artyom Poptsov <poptsov.artyom@gmail.com> skribis:

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

I just realized I was still using 0.9.0, sorry for the confusion!

> 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;
>     }

I think this is incorrect.  AIUI, with 2.0.12, the SMOB type predicate
is always false, so you end up never freeing anything; see smob.c:

--8<---------------cut here---------------start------------->8---
static void
finalize_smob (void *ptr, void *data)
{
  SCM smob;
  scm_t_bits smobnum;
  size_t (* free_smob) (SCM);

  smob = PTR2SCM (ptr);
  smobnum = (scm_t_bits) GC_call_with_alloc_lock (clear_smobnum, ptr);

#if 0
  printf ("finalizing SMOB %p (smobnum: %u)\n", ptr, smobnum);
#endif

  free_smob = scm_smobs[smobnum].free;
  if (free_smob)
    free_smob (smob);
}
--8<---------------cut here---------------end--------------->8---

Here ‘clear_smobnum’ is the procedure that changes PTR to have the
“finalized smob” type.

So the right thing would be to replace:

  struct session_data *data = _scm_to_session_data (session_smob);

… with:

  struct session_data *data = SCM_SMOB_DATA (session_smob);

Does it make sense?

Thanks,
Ludo’.

Re: The “finalized” SMOB type

[Artyom Poptsov]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

Hello Ludovic and David,

thanks for all the comments on the smob freeing callbacks.  Apparently I
misread the documentation so the bug was introduced.  I've changed the
callback procedures as was suggested; the patches included in Guile-SSH
version 0.10.1 released a few moments ago.

- Artyom

-- 
Artyom V. Poptsov <poptsov.artyom@gmail.com>;  GPG Key: 0898A02F
Home page: http://poptsov-artyom.narod.ru/

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]