Emacs core TLS support

[Ted Zlatanov]

On Mon, 06 Sep 2010 23:00:51 +0200 Stefan Monnier <monnier at iro.umontreal.ca> wrote: 

>> In this case I think that's the right approach actually so we
>> shouldn't have defconsts.  See new definition, it uses two local
>> Lisp_Objects for the symbol names.  Where should I allocate those
>> constant Lisp_Objects globally properly?

SM> It's typically declared as global (static or not, depending on whether
SM> it's used in other files) and initialized in syms_of_<foo>.
SM> Look at other syms_of_<foo> to see what it looks like.

Done, thanks.

>> And should I default to anonymous?

SM> I don't know what that means.

If the user passes an unknown symbol to `gnutls-cred-set', should it be
treated as `gnutls_anon' or generate an error?  It could work either
way.  I'm leaning towards an error but it seems kind of rude to the
user.  OTOH it could be a serious problem to use encryption the user did
not intend because of a typo.

SM> the slots you add at the end are ignored by the GC (which is what
SM> you want in your case, since they're not Lisp objects and hence the
SM> GC wouldn't know what to do with them) and can be of any type.  So
SM> just use the types you need here such that casts aren't needed.

OK.  I introduced a new field `gnutls_state_usable' to indicate the
session has been initialized.  I could have made it a byte but it may be
useful to hold Lisp-related state for this patch as it evolves.  It's
before the GC marker field "pid" so it will be noticed by alloc.c.

SM> BTW, if it makes the code simpler, you can use the following trick: use
SM> symbols, but associate an integer to each symbol by way of
SM> symbol properties.

I don't like the properties because they are loosely bound to the symbol
(for errors I think it's better to bind meaning to value tightly).  Is
it OK to do the current approach, where I have the function
`gnutls_make_error' to return the right thing, whether it's a known
integer-as-symbol or a generic integer?  I think it's the right approach
and it seems semantically sensible.  Plus it's easy to extend
`gnutls_make_error' as we need more errors by name.

SM> The type you declare should correspond to the type of the objects you
SM> store there.  Always.  If you stick to this principle and avoid freeing
SM> live objects (and stay within array bounds, and a few more such things)
SM> your code will be more portable and won't dump core (hence will be
SM> generally easier to debug).

Got it.  I think I'm doing it more correctly now and there will be no GC
issues, as I mentioned above.

On Mon, 06 Sep 2010 19:18:01 +0200 Andreas Schwab <schwab at linux-m68k.org> wrote: 

AS> Ted Zlatanov <tzz at lifelogs.com> writes:

>> +HAVE_GNUTLS=no
>> +if test "${with_gnutls}" = "yes" ; then
>> +  PKG_CHECK_MODULES([LIBGNUTLS], [gnutls >= 2.2.4])

AS> Are you sure you want to make gnutls a required dependency of Emacs?

>> +  AC_DEFINE(HAVE_GNUTLS)

AS> $ autoreconf
AS> autoheader: warning: missing template: HAVE_GNUTLS
AS> autoheader: Use AC_DEFINE([HAVE_GNUTLS], [], [Description])
AS> autoreconf: /usr/bin/autoheader failed with exit status: 1

No.  What would you suggest?

On Mon, 06 Sep 2010 17:53:46 +0200 Andreas Schwab <schwab at linux-m68k.org> wrote: 

AS> Ted Zlatanov <tzz at lifelogs.com> writes:
>>>> +DEFUN ("gnutls-init", Fgnutls_init, Sgnutls_init, 2, 2, 0,
>> ...
>>>> +  ret = gnutls_init((gnutls_session_t*)&(XPROCESS(proc)->gnutls_state), 
>> 
AS> Aliasing violation.
>> 
>> Can you explain please?

AS> The function wants to store a value of one type into an object of a
AS> different type.  BAD.  The compiler is allowed to assume the object was
AS> never changed.

OK, you mean the cast is wrong.  I fixed that.  That leaves only the
transport cast from int in gnutls_{handshake,rehandshake} which I
believe is right from the original patch.

AS> IMHO all your functions should return t on success and either some error
AS> symbol on failure or even raise an error.
>> 
>> Yes, but I'm not sure which one.  Can you recommend?

AS> Take your pick.  I don't know anything about gnutls.

Well, none of the failures are fatal and there's a lot of ways to retry
the connection.  I think it's better to return the integer error value
or t to simplify the usage.  I changed the patch accordingly.

>>>> === modified file 'src/process.h'
>>>> +
>>>> +#ifdef HAVE_GNUTLS
>>>> +    /* XXX Store GNU TLS state and auth mechanisms in Lisp_Objects. */
>>>> +    Lisp_Object gnutls_state;
>>>> +    Lisp_Object x509_cred, x509_callback;
>>>> +    Lisp_Object anon_cred;
>>>> +    Lisp_Object srp_cred;
>>>> +#endif
>> 
AS> None of them should be Lisp_Objects.  Also make sure the resources are
AS> properly released when the process object is deleted.
>> 
>> I don't know enough (the choice of using Lisp_Objects was in the
>> original patch) to know what to do instead of using Lisp_Objects.  Why
>> not, first of all?

AS> You never store Lisp_Object values in there, so what's the point?
AS> x509_callback is never used, btw.

Fixed (also see my response above to Stefan Monnier).  I've attached the
patch as it stands.

Thanks again for all your comments.  Getting there...

Ted

-------------- next part --------------
A non-text attachment was scrubbed...
Name: tls.patch
Type: text/x-diff
Size: 24470 bytes
Desc: not available
URL: </pipermail/attachments/20100906/9667e273/attachment.patch>