[Pkg-silc-devel] Bug#482145: silc client segfaults if key is verified after disconnection

[Skywing]

This sounds reasonable enough (still has the problem where we can't safely unload the .so containing the callback, but don't think that is realistically getting fixed at this point).

I'll see about providing you with a patch that fixes this using the suggested method + does the additional work (above and beyond the SilcAsyncOperation wrappering) required to fix /getkey.

- S

-----Original Message-----
From: Pekka Riikonen [mailto:priikone at iki.fi]
Sent: Saturday, July 12, 2008 5:46 AM
To: Skywing
Cc: Jérémy Bobbio; silc-devel at lists.silcnet.org; 482145 at bugs.debian.org
Subject: RE: silc client segfaults if key is verified after disconnection

On Fri, 11 Jul 2008, Skywing wrote:

: All of the key confirmation prompts suffer from this flaw.  I've got a
: fix for /getkey being broken, other than the lack of reference counting
: for the SilcClient and SilcClientConnection objects.  There is nothing
: that can really be done about the SilcClient object not being refcounted
:
Both of you have stumbled to the fundamental problem of most asynchronous
operations; the inability to control them.  That's why SILC's runtime
provides the SILC Async Operation API (from 1.2 docs:
http://srt.silc.fi/silcasync_hsilcutil2FAsync20Operation20Interface.html)

Here in a psuedo-code fashion is one way to fix this and all other prompt
problems in SILC Client:

In this particular problem reported by Jérémy we can fix this by wrapping
the keyboard_entry_redirect call to our own call that uses the
SilcAyncOperation:

SilcAsyncOperation
my_keyboard_entry_redirect(SIGNAL_FUNC func,
                           SilcAsyncOperationAbort abort_callback,
                           const char *entry, int flags, void *data)
{
  SilcAsyncOperation op = silc_async_alloc(abort_callback, NULL, data);
  keyboard_entry_redirect(func, format, flags, data);
  return op;
}

The returned op would be saved to some location where it is globally
available, say, inside SILC_SERVER_REC.  Some other context could be used
too, but just for the purpose of this example let's use that.

To present user with the key verification prompt, we now would call:

  server->prompt_op =
    my_keyboard_entry_redirect(verify_public_key_completion,
                               verify_public_key_abort,
                               format, 0, verify);

If the server connection is now closed or whatever happens that would
invalidate the data inside the structure we used with the call above, we
now call in whatever callback client library called to indicate the
disconnection:

  silc_async_abort(server->prompt_op, NULL, NULL);

This will call the verify_public_key_abort which must now mark the
operation aborted:

void verify_public_key_abort(SilcAsyncOperation op, void *context)
{
  PublicKeyVerify verify = context;
  verify->aborted = TRUE;
}

When the verify_public_key_completion is finally called when user actually
answers the prompt, we check:

  if (verify->aborted) {
    /* We was aborted, do nothing */
    silc_free(verify);
    server->prompt_op = NULL;
    return;
  }

  /* We wasn't aborted, free operation and proceed */
  silc_async_free(server->prompt_op);
  server->prompt_op = NULL;

NULLing the operation is important to avoid using it accidentally.  Of
course the operation context must be saved into some place that itself
cannot be freed underneath the async call.  I'm not sure SILC_SERVE_REC is
the correct place even though I used it in this example.

The SILC Client Library should use SilcAsyncOperations with all client ops
it calls to application that take completion callbacks as argument so that
the library itself could abort the client ops.  This however would require
changing the SilcClientOperations API...

        Pekka
________________________________________________________________________
 Pekka Riikonen                                 priikone at silcnet.org
 Secure Internet Live Conferencing (SILC)       http://silcnet.org/