From debbugs-submit-bounces@debbugs.gnu.org Mon May 29 17:27:05 2017 Received: (at 26948) by debbugs.gnu.org; 29 May 2017 21:27:05 +0000 Received: from localhost ([127.0.0.1]:43881 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dFSBs-0003Rp-RL for submit@debbugs.gnu.org; Mon, 29 May 2017 17:27:05 -0400 Received: from world.peace.net ([50.252.239.5]:56467) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dFSBq-0003RH-Cl for 26948@debbugs.gnu.org; Mon, 29 May 2017 17:27:02 -0400 Received: from pool-72-93-28-22.bstnma.east.verizon.net ([72.93.28.22] helo=jojen) by world.peace.net with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1dFSBj-0000Tq-UX; Mon, 29 May 2017 17:26:56 -0400 From: Mark H Weaver To: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: bug#26948: gnutls errors on multiple guix commands References: <8737c51e6r.fsf@gmail.com> <87shk3y74g.fsf@gnu.org> <8737btieie.fsf@gmail.com> <87vaoovvvz.fsf@gnu.org> <87poes25dw.fsf@netris.org> <87a85wc8li.fsf@gnu.org> Date: Mon, 29 May 2017 17:26:43 -0400 In-Reply-To: <87a85wc8li.fsf@gnu.org> ("Ludovic \=\?utf-8\?Q\?Court\=C3\=A8s\=22'\?\= \=\?utf-8\?Q\?s\?\= message of "Mon, 29 May 2017 11:31:37 +0200") Message-ID: <87a85v1hik.fsf@netris.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Score: 0.0 (/) X-Debbugs-Envelope-To: 26948 Cc: 26948@debbugs.gnu.org, Maxim Cournoyer X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: debbugs-submit-bounces@debbugs.gnu.org Sender: "Debbugs-submit" X-Spam-Score: 0.0 (/) Hi Ludovic, ludo@gnu.org (Ludovic Court=C3=A8s) writes: > Mark H Weaver skribis: > >> This reminds me of a bug that I found in the Guile binding in GnuTLS a >> while ago, but forgot to report. Maybe it's related: >> >> In 'set_certificate_file' in gnutls-3.5.9/guile/src/core.c: >> >> static unsigned int >> set_certificate_file (certificate_set_file_function_t set_file, >> SCM cred, SCM file, SCM format, const char *func= _name) >> #define FUNC_NAME func_name >> { >> int err; >> char *c_file; >> size_t c_file_len; >>=20=20=20 >> gnutls_certificate_credentials_t c_cred; >> gnutls_x509_crt_fmt_t c_format; >>=20=20=20 >> c_cred =3D scm_to_gnutls_certificate_credentials (cred, 1, FUNC_NAME= ); >> SCM_VALIDATE_STRING (2, file); >> c_format =3D scm_to_gnutls_x509_certificate_format (format, 3, FUNC_= NAME); >>=20=20=20 >> c_file_len =3D scm_c_string_length (file); >> c_file =3D alloca (c_file_len + 1); >>=20=20=20 >> (void) scm_to_locale_stringbuf (file, c_file, c_file_len + 1); >> c_file[c_file_len] =3D '\0'; >>=20=20=20 >> err =3D set_file (c_cred, c_file, c_format); >> if (EXPECT_FALSE (err < 0)) >> scm_gnutls_error (err, FUNC_NAME); >>=20=20=20 >> /* Return the number of certificates processed. */ >> return ((unsigned int) err); >> } >> >> 'scm_c_string_length' is inappropriately assumed to return the length >> of the encoded C string in bytes, whereas it actually returns the >> number of characters (code points). > > This is terrible! WDYT of this: > > diff --git a/guile/src/core.c b/guile/src/core.c > index 605c91f7a..38d573fa9 100644 > --- a/guile/src/core.c > +++ b/guile/src/core.c > @@ -1,5 +1,5 @@ > /* GnuTLS --- Guile bindings for GnuTLS. > - Copyright (C) 2007-2014, 2016 Free Software Foundation, Inc. > + Copyright (C) 2007-2014, 2016-2017 Free Software Foundation, Inc. >=20=20 > GnuTLS is free software; you can redistribute it and/or > modify it under the terms of the GNU Lesser General Public > @@ -1428,22 +1428,19 @@ set_certificate_file (certificate_set_file_functi= on_t set_file, > { > int err; > char *c_file; > - size_t c_file_len; >=20=20 > gnutls_certificate_credentials_t c_cred; > gnutls_x509_crt_fmt_t c_format; >=20=20 > c_cred =3D scm_to_gnutls_certificate_credentials (cred, 1, FUNC_NAME); > SCM_VALIDATE_STRING (2, file); > - c_format =3D scm_to_gnutls_x509_certificate_format (format, 3, FUNC_NA= ME); > - > - c_file_len =3D scm_c_string_length (file); > - c_file =3D alloca (c_file_len + 1); >=20=20 > - (void) scm_to_locale_stringbuf (file, c_file, c_file_len + 1); > - c_file[c_file_len] =3D '\0'; > + c_format =3D scm_to_gnutls_x509_certificate_format (format, 3, FUNC_NA= ME); > + c_file =3D scm_to_locale_string (file); >=20=20 > err =3D set_file (c_cred, c_file, c_format); > + free (c_file); > + > if (EXPECT_FALSE (err < 0)) > scm_gnutls_error (err, FUNC_NAME); >=20=20 Looks good to me. In the case when a UTF-8 locale is active, and where Guile 2.0.12 or later is available, we could use 'scm_c_string_utf8_length' to find the length in bytes, but optimizing that case is probably not worth the extra code complexity. > Unfortunately there=E2=80=99s a dozen of places in core.c that use this i= diom > and would need to be fixed (it=E2=80=99s pre-2.0 code I think). Bummer. > In the meantime we can work around it this way: > > diff --git a/guix/build/download.scm b/guix/build/download.scm > index ce4708a87..6ef623334 100644 > --- a/guix/build/download.scm > +++ b/guix/build/download.scm > @@ -296,6 +296,13 @@ session record port using PORT as its underlying com= munication port." > (make-parameter (or (getenv "GUIX_TLS_CERTIFICATE_DIRECTORY") > (getenv "SSL_CERT_DIR")))) ;like OpenSSL >=20=20 > +(define (set-certificate-credentials-x509-trust-file!* cred file format) > + "Like 'set-certificate-credentials-x509-trust-file!', but without the = file > +name decoding bug described at > +." > + (let ((data (call-with-input-file file get-bytevector-all))) > + (set-certificate-credentials-x509-trust-data! cred data format))) > + > (define (make-credendials-with-ca-trust-files directory) > "Return certificate credentials with X.509 authority certificates read= from > DIRECTORY. Those authority certificates are checked when > @@ -309,7 +316,7 @@ DIRECTORY. Those authority certificates are checked = when > (let ((file (string-append directory "/" file))) > ;; Protect against dangling symlinks. > (when (file-exists? file) > - (set-certificate-credentials-x509-trust-file! > + (set-certificate-credentials-x509-trust-file!* > cred file > x509-certificate-format/pem)))) > (or files '())) > > > WDYT? I=E2=80=99ll commit it if that=E2=80=99s fine with you. I'm not sufficiently familiar with GnuTLS to properly review this, but I trust your judgement. Thanks! Mark