Re: master 583995c: GnuTLS HMAC and symmetric cipher support

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Teodor Zlatanov wrote:

> branch: master
> commit 583995c62dd424775dda33d5134ce04bee2ae685

Hi, this change apparently causes gnutls tests to segfault on hydra.
I can't give you any more information than is accessible from

http://hydra.nixos.org/build/56430842

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Ted Zlatanov]

On Sun, 16 Jul 2017 20:05:15 -0400 Glenn Morris <rgm@gnu.org> wrote: 

GM> Teodor Zlatanov wrote:
>> branch: master
>> commit 583995c62dd424775dda33d5134ce04bee2ae685

GM> Hi, this change apparently causes gnutls tests to segfault on hydra.
GM> I can't give you any more information than is accessible from

GM> http://hydra.nixos.org/build/56430842

Thanks, Glenn. The logs have nothing useful unfortunately[1].

I'm not looking to install Nix on my personal machine to debug this,
which seems to be necessary in order to replicate the issue. I also
don't have a login to any machines that can run Nix. Can you or anyone
else recommend a way to replicate the issue using a Docker or LXC
container or other means? Or give me a login to the Hydra build farm?

Thanks
Ted

[1] The log view link in Hydra doesn't work with eww. That's not a big
deal but I thought I'd mention it.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Michael Albinus]

Ted Zlatanov <tzz@lifelogs.com> writes:

Hi Ted,

> Thanks, Glenn. The logs have nothing useful unfortunately[1].

For Tramp, I have modified the Makefile to get continuous print of the
log to stdout. This helps in cases Emacs crashes (your case) or blocks
(Tramp case).

We could extend this for GnuTLS tests. Ping me if you want me to do
this (or check my changes in test/Makefile.in).

> Thanks
> Ted

Best regards, Michael.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Ted Zlatanov]

On Mon, 17 Jul 2017 16:30:38 +0200 Michael Albinus <michael.albinus@gmx.de> wrote: 

MA> For Tramp, I have modified the Makefile to get continuous print of the
MA> log to stdout. This helps in cases Emacs crashes (your case) or blocks
MA> (Tramp case).

MA> We could extend this for GnuTLS tests. Ping me if you want me to do
MA> this (or check my changes in test/Makefile.in).

I'd love this. Maybe there could be a way of triggering this mode from
Hydra's interface, so it's not always on. If not, then yes, please make
the change. I assume it will be related to

    WRITE_LOG = $(if $(and ${NIX_STORE}, $(findstring tramp, $@)), |& tee $@, > $@ 2>&1) \

but a $(or) call just for GnuTLS would duplicate quite a bit of code,
and $(findstring) doesn't do regexes. Maybe you can decide :)

Thanks again
Ted

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

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


Please stop dropping me from the cc. :(

Ted Zlatanov wrote:

>>> branch: master
>>> commit 583995c62dd424775dda33d5134ce04bee2ae685
>
> GM> Hi, this change apparently causes gnutls tests to segfault on hydra.
> GM> I can't give you any more information than is accessible from
>
> GM> http://hydra.nixos.org/build/56430842
>
> Thanks, Glenn. The logs have nothing useful unfortunately[1].

The logs say that gnutls 3.2.21 was used.
I installed that version on rhel 7, and trivially reproduced the crash
when loading the gnutls tests. Backtrace attached. Maybe if you install
that version, it will crash for you too...


[-- Attachment #2: gdb.txt.xz --]
[-- Type: application/octet-stream, Size: 5428 bytes --]

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Noam Postavsky]

On Mon, Jul 17, 2017 at 1:12 PM, Glenn Morris <rgm@gnu.org> wrote:
>
> The logs say that gnutls 3.2.21 was used.

How do you find this info from the logs? I searched for "gnutls" and
found only stuff like:

checking for LIBGNUTLS... yes
checking for LIBGNUTLS3... yes

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Noam Postavsky wrote:

> On Mon, Jul 17, 2017 at 1:12 PM, Glenn Morris <rgm@gnu.org> wrote:
>>
>> The logs say that gnutls 3.2.21 was used.
>
> How do you find this info from the logs?

Sorry, the "build dependencies" tab, not the actual build log.

The crash occurs when gca = GNUTLS_CIPHER_UNKNOWN and
gnutls_cipher_get_name = NULL.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Michael Albinus]

Ted Zlatanov <tzz@lifelogs.com> writes:

> I'd love this. Maybe there could be a way of triggering this mode from
> Hydra's interface, so it's not always on. If not, then yes, please make
> the change.

I'll wait a little bit, whether you could reproduce the problem with
Glenn's hint.

> Thanks again
> Ted

Best regards, Michael.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Noam Postavsky]

On Mon, Jul 17, 2017 at 2:20 PM, Glenn Morris <rgm@gnu.org> wrote:
> Noam Postavsky wrote:
>
>> On Mon, Jul 17, 2017 at 1:12 PM, Glenn Morris <rgm@gnu.org> wrote:
>>>
>>> The logs say that gnutls 3.2.21 was used.
>>
>> How do you find this info from the logs?
>
> Sorry, the "build dependencies" tab, not the actual build log.

Oh, this is confusing; the build that you linked to,
http://hydra.nixos.org/build/56430842, doesn't have a "build
dependencies" tab.

Although http://hydra.nixos.org/build/56475235 for example, does.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Noam Postavsky wrote:

> http://hydra.nixos.org/build/56430842, doesn't have a "build
> dependencies" tab.
>
> Although http://hydra.nixos.org/build/56475235 for example, does.

Oh, weird. Maybe this information gets garbage-collected fairly
rapidly to save space. Just guessing though.


After fixing the crash issue, test-gnutls-004-symmetric-ciphers fails with:

  (error "GnuTLS cipher IDEA-PGP-CFB/encrypt initialization failed: The
  request is invalid.")


BTW I note that gnutls_mac_get_name and gnutls_digest_get_name can
also apparently return NULL, and are currently passed to "intern"
without checking for this. I don't know if this can actually crash in
practice though.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Paul Eggert]

On 07/17/2017 12:27 PM, Glenn Morris wrote:
> gnutls_mac_get_name and gnutls_digest_get_name can
> also apparently return NULL

As I recall, they can't return NULL in the contexts where they're used, 
as a previous call already established the validities of their arguments.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Paul Eggert wrote:

>> gnutls_mac_get_name and gnutls_digest_get_name can
>> also apparently return NULL
>
> As I recall, they can't return NULL in the contexts where they're
> used, as a previous call already established the validities of their
> arguments.

It's not obvious to me that 2 and 3 below are any different to 1, save
for the fact that GNUTLS_CIPHER_NULL == 1 (and GNUTLS_CIPHER_UNKNOWN ==
0, which is maybe what happens to stop 2 and 3 crashing in the same way
1 does. Seems a bit like a happy accident?):


1. Fgnutls_ciphers. Can intern NULL and crash.
const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
{
  gnutls_cipher_algorithm_t gca = gciphers[pos];
  Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
  ...
}


2. Fgnutls_macs.
const gnutls_mac_algorithm_t *macs = gnutls_mac_list ();
for (ptrdiff_t pos = 0; macs[pos] != 0; pos++)
{
  const gnutls_mac_algorithm_t gma = macs[pos];
  Lisp_Object gma_symbol = intern (gnutls_mac_get_name (gma));
  ...
}


3. Fgnutls_digests.
const gnutls_digest_algorithm_t *digests = gnutls_digest_list ();
for (ptrdiff_t pos = 0; digests[pos] != 0; pos++)
{
  const gnutls_digest_algorithm_t gda = digests[pos];
  Lisp_Object gda_symbol = intern (gnutls_digest_get_name (gda));
  ...
}

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Paul Eggert]

Glenn Morris wrote:

> Seems a bit like a happy accident?):

It's not an accident, as every valid cipher etc. has a name.

> 1. Fgnutls_ciphers. Can intern NULL and crash.
> const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
> for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
> {
>    gnutls_cipher_algorithm_t gca = gciphers[pos];
>    Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
>    ...
> }

It's OK, as every cipher other than GNUTLS_CIPHER_NULL has a non-NULL name.

> 2. Fgnutls_macs.
> const gnutls_mac_algorithm_t *macs = gnutls_mac_list ();
> for (ptrdiff_t pos = 0; macs[pos] != 0; pos++)
> {
>    const gnutls_mac_algorithm_t gma = macs[pos];
>    Lisp_Object gma_symbol = intern (gnutls_mac_get_name (gma));
>    ...
> }

Also OK, as every mac algorithm has a non-NULL name.

> 3. Fgnutls_digests.
> const gnutls_digest_algorithm_t *digests = gnutls_digest_list ();
> for (ptrdiff_t pos = 0; digests[pos] != 0; pos++)
> {
>    const gnutls_digest_algorithm_t gda = digests[pos];
>    Lisp_Object gda_symbol = intern (gnutls_digest_get_name (gda));
>    ...
> }

Likewise.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Paul Eggert wrote:

> Glenn Morris wrote:
>
>> Seems a bit like a happy accident?):
>
> It's not an accident, as every valid cipher etc. has a name.
>
>> 1. Fgnutls_ciphers. Can intern NULL and crash.
>> const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
>> for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
>> {
>>    gnutls_cipher_algorithm_t gca = gciphers[pos];
>>    Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
>>    ...
>> }
>
> It's OK, as every cipher other than GNUTLS_CIPHER_NULL has a non-NULL name.

But it isn't OK. That's what this thread is about.
GNUTLS_CIPHER_UNKNOWN apparently has a NULL name.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Paul Eggert]

Glenn Morris wrote:
>> It's OK, as every cipher other than GNUTLS_CIPHER_NULL has a non-NULL name.
> But it isn't OK. That's what this thread is about.
> GNUTLS_CIPHER_UNKNOWN apparently has a NULL name.

Sorry, I still don't see the problem here. From what I can see, 
GNUTLS_CIPHER_NULL is not a cipher (or to be more precise, it is not a valid 
GnuTLS cipher ID). The code in question:

> const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
> for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
> {
>   gnutls_cipher_algorithm_t gca = gciphers[pos];
>   Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));

looks only at GnuTLS cipher IDs returned by gnutls_cipher_list, and 
GNUTLS_CIPHER_UNKNOWN is not one of those cipher IDs.

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Paul Eggert wrote:

> looks only at GnuTLS cipher IDs returned by gnutls_cipher_list, and
> GNUTLS_CIPHER_UNKNOWN is not one of those cipher IDs.

I feel like you've missed the first half of the discussion, so I'll
summarize it:

Since 583995c, make check on hydra crashes when loading the gnutls tests.
It uses gnutls 3.2.21. I installed that on my rhel7 system (with
--disable-non-suiteb-curves) and reproduced the crash. The backtrace is
at http://lists.gnu.org/archive/html/emacs-devel/2017-07/msg00716.html .

Specifically, I find it crashes because gnutls_cipher_list returns a list
containing GNUTLS_CIPHER_UNKNOWN.

The following patch fixes it for me:

*** /tmp/uOhC3f_gnutls.c	2017-07-17 19:32:32.851361998 -0700
--- src/gnutls.c	2017-07-17 12:03:27.514906186 -0700
***************
*** 1857,1870 ****
    for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
      {
        gnutls_cipher_algorithm_t gca = gciphers[pos];
  
        /* A symbol representing the GnuTLS cipher.  */
!       Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
  
!       ptrdiff_t cipher_tag_size = gnutls_cipher_get_tag_size (gca);
  
!       Lisp_Object cp
! 	= listn (CONSTYPE_HEAP, 15, cipher_symbol,
  		 QCcipher_id, make_number (gca),
  		 QCtype, Qgnutls_type_cipher,
  		 QCcipher_aead_capable, cipher_tag_size == 0 ? Qnil : Qt,
--- 1857,1877 ----
    for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
      {
        gnutls_cipher_algorithm_t gca = gciphers[pos];
+       const char *cipher_name;
+       Lisp_Object cipher_symbol, cp;
+       ptrdiff_t cipher_tag_size;
+ 
+       cipher_name = gnutls_cipher_get_name (gca);
+ 
+       if (! cipher_name)
+         continue;
  
        /* A symbol representing the GnuTLS cipher.  */
!       cipher_symbol = intern (cipher_name);
  
!       cipher_tag_size = gnutls_cipher_get_tag_size (gca);
  
!       cp = listn (CONSTYPE_HEAP, 15, cipher_symbol,
                    QCcipher_id, make_number (gca),
                    QCtype, Qgnutls_type_cipher,
                    QCcipher_aead_capable, cipher_tag_size == 0 ? Qnil : Qt,

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Paul Eggert]

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

Glenn Morris wrote:
> Since 583995c, make check on hydra crashes when loading the gnutls tests.
> It uses gnutls 3.2.21. I installed that on my rhel7 system (with
> --disable-non-suiteb-curves) and reproduced the crash. The

I don't see how the crash can occur with vanilla GnuTLS 3.2.21, as its 
gnutls_cipher_list returns a list of IDs that does not contain 
GNUTLS_CIPHER_UNKNOWN. Perhaps you were using a modified GnuTLS 3.2.21. Or 
possibly I'm misreading the GnuTLS source code, though I don't see how.

As you're observing the problem I installed the attached, which is similar to 
the patch that worked for you.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Port-gnutls.c-to-older-buggier-GnuTLS.patch --]
[-- Type: text/x-patch; name="0001-Port-gnutls.c-to-older-buggier-GnuTLS.patch", Size: 1693 bytes --]

From 376151481b2172dbb08d25bb5946f0f627f7453d Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 18 Jul 2017 00:37:03 -0700
Subject: [PATCH] Port gnutls.c to older (buggier?) GnuTLS

Problem reported for GnuTLS 3.2.1 by Glenn Morris in:
http://lists.gnu.org/archive/html/emacs-devel/2017-07/msg00716.html
http://lists.gnu.org/archive/html/emacs-devel/2017-07/msg00742.html
Although I don't see how this bug can occur with vanilla GnuTLS 3.2.1,
perhaps hydra was using a modified GnuTLS.
* src/gnutls.c (Fgnutls_ciphers): Don't assume GNUTLS_CIPHER_NULL
is at the end of the list returned by gnutls_cipher_list,
or that the earlier ciphers all have non-null names.
---
 src/gnutls.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index 9fbaea2..e406d66 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -1854,12 +1854,17 @@ The alist key is the cipher name. */)
 
 #ifdef HAVE_GNUTLS3_CIPHER
   const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
-  for (ptrdiff_t pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
+  for (ptrdiff_t pos = 0; gciphers[pos] != 0; pos++)
     {
       gnutls_cipher_algorithm_t gca = gciphers[pos];
+      if (gca == GNUTLS_CIPHER_NULL)
+	continue;
+      char const *cipher_name = gnutls_cipher_get_name (gca);
+      if (!cipher_name)
+	continue;
 
       /* A symbol representing the GnuTLS cipher.  */
-      Lisp_Object cipher_symbol = intern (gnutls_cipher_get_name (gca));
+      Lisp_Object cipher_symbol = intern (cipher_name);
 
       ptrdiff_t cipher_tag_size = gnutls_cipher_get_tag_size (gca);
 
-- 
2.7.4

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Paul Eggert wrote:

> I don't see how the crash can occur with vanilla GnuTLS 3.2.21, as its
> gnutls_cipher_list returns a list of IDs that does not contain
> GNUTLS_CIPHER_UNKNOWN. Perhaps you were using a modified GnuTLS
> 3.2.21. Or possibly I'm misreading the GnuTLS source code, though I
> don't see how.

I was using ftp://ftp.gnutls.org/gcrypt/gnutls/v3.2/gnutls-3.2.21.tar.xz

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Paul Eggert]

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

Glenn Morris wrote:
> I was using ftp://ftp.gnutls.org/gcrypt/gnutls/v3.2/gnutls-3.2.21.tar.xz

Weird, because I was looking at the same source code. It does not build under 
recent Ubuntu or Fedora, so I did not compile it: I just looked at the source 
code. Its implementation of gnutls_cipher_list appears to be straightforward: it 
adds only values found as the 'id' component of an entry in the 'algorithms' 
array in lib/algorithms/ciphers.c. None of these values are 
GNUTLS_CIPHER_UNKNOWN, so gnutls_cipher_list cannot possibly return an array 
containing GNUTLS_CIPHER_UNKNOWN.

For example, on my platform, if I compile and run the attached program via:

gcc gs.c -lgnutls
./a.out

the program silently succeeds. What happens on your platform?

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gs.c --]
[-- Type: text/x-csrc; name="gs.c", Size: 387 bytes --]

#include <gnutls/gnutls.h>
#include <stdio.h>

int
main (void)
{
  const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
  for (int pos = 0; gciphers[pos] != 0; pos++)
    {
      gnutls_cipher_algorithm_t gca = gciphers[pos];
      if (gca == GNUTLS_CIPHER_UNKNOWN)
	return printf ("gciphers[%d] == GNUTLS_CIPHER_UNKNOWN ?!?\n", pos), 1;
    }
  return 0;
}

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Paul Eggert wrote:

> For example, on my platform, if I compile and run the attached program via:
>
> gcc gs.c -lgnutls
> ./a.out
>
> the program silently succeeds. What happens on your platform?

Your example doesn't correspond to how the code in Emacs used to be.
Change it to:

  for (int pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)

and it fails for me, just as Emacs used to:

  gciphers[17] == GNUTLS_CIPHER_UNKNOWN ?!?

As I said, it seems to be a happy accident that GNUTLS_CIPHER_UNKNOWN == 0.
So your example as written happens to skip it.

I hope we have now established I'm not mad or lying... :)

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Paul Eggert]

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

Glenn Morris wrote:
> Your example doesn't correspond to how the code in Emacs used to be.
> Change it to:
> 
>    for (int pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
> 
> and it fails for me, just as Emacs used to:

Weirder and weirder. First, with that change the code still works for me (I 
tried it with GnuTLS 3.4.10). Second, if I read the GnuTLS 3.2.21 source code, 
the changed code should still work for you too. This is because the test 
gciphers[pos] != GNUTLS_CIPHER_NULL should succeed one iteration before the test 
gciphers[pos] != 0 succeeds.

> 
>    gciphers[17] == GNUTLS_CIPHER_UNKNOWN ?!?
> 
> As I said, it seems to be a happy accident that GNUTLS_CIPHER_UNKNOWN == 0.
> So your example as written happens to skip it.
> 
> I hope we have now established I'm not mad or lying... :)

Sorry, not established yet. :-) Quite possibly I'm the crazy one, of course. 
Still, there's something very odd going on here.

To make sure we're on the same page I am attaching the changed code, which still 
works for me.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: gs1.c --]
[-- Type: text/x-csrc; name="gs1.c", Size: 404 bytes --]

#include <gnutls/gnutls.h>
#include <stdio.h>

int
main (void)
{
  const gnutls_cipher_algorithm_t *gciphers = gnutls_cipher_list ();
  for (int pos = 0; gciphers[pos] != GNUTLS_CIPHER_NULL; pos++)
    {
      gnutls_cipher_algorithm_t gca = gciphers[pos];
      if (gca == GNUTLS_CIPHER_UNKNOWN)
	return printf ("gciphers[%d] == GNUTLS_CIPHER_UNKNOWN ?!?\n", pos), 1;
    }
  return 0;
}

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Glenn Morris]

Paul Eggert wrote:

> To make sure we're on the same page I am attaching the changed code,
> which still works for me.

For the Nth time:

When compiled and run using vanilla gnutls 3.2.21, this code prints

  gciphers[17] == GNUTLS_CIPHER_UNKNOWN ?!?

Re: master 583995c: GnuTLS HMAC and symmetric cipher support

[Ted Zlatanov]

On Mon, 17 Jul 2017 13:12:27 -0400 Glenn Morris <rgm@gnu.org> wrote: 

GM> The logs say that gnutls 3.2.21 was used.
GM> I installed that version on rhel 7, and trivially reproduced the crash
GM> when loading the gnutls tests. Backtrace attached. Maybe if you install
GM> that version, it will crash for you too...

Hi Glenn,

the build appears fixed now, thanks to the fixes that Paul kindly
applied. Am I correct or am I misinterpreting the Hydra build report?

Would it be possible to have Hydra also test the latest release and
latest Git checkout of the GnuTLS library?

GM> Please stop dropping me from the cc. :(

It really is easier for me to use GMane (as a followup to the list
only), but I understand it's an inconvenience for you and will try to
adjust accordingly.

Ted

P.S. Sorry I couldn't reply sooner, I am traveling abroad for family
reasons.