bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Laurent Charignon]

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

#### Context

The symptom of this issue first came up in the context of `ghub`, a package
that offers interfacing with Github from emacs (issue:
https://github.com/magit/ghub/issues/81).

Multiple users of the `ghub` package started observing that
synchronously retrieving content from the github API through ghub
returned blank responses.

I investigated and narrowed down a minimal repro case of the problem,
which I believe indicate that the problem is in Emacs itself.

#### Steps to reproduce

1) Run emacs -Q
2) Evaluate the following elisp snippet
```
(switch-to-buffer (url-retrieve-synchronously "
https://api.github.com/users/charignon/repos")
                  (buffer-string))
```

I expect then to see a buffer with an http response, instead I get a
blank buffer.

#### Investigation / Observation

1) This behavior reproduces consistently with api.github.com, not with
any websites that I tried.

2) `url-retrieve`, the asynchronous way to fetch url, does not seem to be
subject to the issue, and works as expected, for
example:
```
(url-retrieve "https://api.github.com/users/charignon/repos" (lambda (x
&rest v) x))
```
works

3) Once a connection is established (for example by `url-retrieve`) and
can be reused, then `url-retrieve-synchronously` works. That is, the
snippet above returns a buffer with an HTTP response from github. Removing
the connection (emacs process managing it) is necessary after you ran
`url-retrieve` if you want to reproduce the bug.

4) I used wireshark to look at the network traffic:

Here is the trace for the `url-retrieve-synchronously` call to github
(from steps to reproduce):

I blurred out the info field for DNS packets (not relevant)

```
No.     Time           Source                Destination           Protocol
Length Info
      9 1.658218501    vulture.lan           testwifi.here         DNS
74     XXX
     10 1.658260266    vulture.lan           testwifi.here         DNS
74     XXX
     11 1.665053481    testwifi.here         vulture.lan           DNS
158    XXX
     12 1.703946584    testwifi.here         vulture.lan           DNS
106    XXX
     13 1.704485426    vulture.lan           api.github.com        TCP
74     55424 → https(443) [SYN] Seq=0 Win=29200 Len=0 MSS=1460 SACK_PERM=1
TSval=2499281666 TSecr=0 WS=128
     14 1.728009793    api.github.com        vulture.lan           TCP
74     https(443) → 55424 [SYN, ACK] Seq=0 Ack=1 Win=28480 Len=0 MSS=1436
SACK_PERM=1 TSval=3029633168 TSecr=2499281666 WS=1024
     15 1.728074018    vulture.lan           api.github.com        TCP
66     55424 → https(443) [ACK] Seq=1 Ack=1 Win=29312 Len=0
TSval=2499281689 TSecr=3029633168
     16 1.748051225    vulture.lan           api.github.com        TLSv1.3
583    Client Hello
     17 1.773082726    api.github.com        vulture.lan           TLSv1.3
1490   Server Hello, Change Cipher Spec, Encrypted Extensions
     18 1.773104270    vulture.lan           api.github.com        TCP
66     55424 → https(443) [ACK] Seq=518 Ack=1425 Win=32128 Len=0
TSval=2499281734 TSecr=3029633179
     19 1.773405428    vulture.lan           api.github.com        TLSv1.3
72     Change Cipher Spec
     20 1.774050525    api.github.com        vulture.lan           TCP
1490   https(443) → 55424 [ACK] Seq=1425 Ack=518 Win=29696 Len=1424
TSval=3029633179 TSecr=2499281709 [TCP segment of a reassembled PDU]
     21 1.775039404    api.github.com        vulture.lan           TLSv1.3
615    Certificate, Certificate Verify, Finished
     22 1.775047977    vulture.lan           api.github.com        TCP
66     55424 → https(443) [ACK] Seq=524 Ack=3398 Win=37760 Len=0
TSval=2499281736 TSecr=3029633179
     23 1.838313183    api.github.com        vulture.lan           TCP
66     https(443) → 55424 [ACK] Seq=3398 Ack=524 Win=29696 Len=0
TSval=3029633196 TSecr=2499281735
     24 1.838358258    vulture.lan           api.github.com        HTTP
 388    GET /users/charignon/repos HTTP/1.1
     25 1.861920124    api.github.com        vulture.lan           TCP
66     https(443) → 55424 [ACK] Seq=3398 Ack=846 Win=30720 Len=0
TSval=3029633201 TSecr=2499281800
     26 1.862284763    api.github.com        vulture.lan           TLSv1.3
145    New Session Ticket
     27 1.862648197    api.github.com        vulture.lan           TLSv1.3
145    New Session Ticket
     28 1.864591679    vulture.lan           api.github.com        TCP
66     55424 → https(443) [RST, ACK] Seq=846 Ack=3556 Win=37760 Len=0
TSval=2499281826 TSecr=3029633201
     29 1.866733948    vulture.lan           testwifi.here         DNS
74     XXX
     30 1.866762442    vulture.lan           testwifi.here         DNS
74     XXX
     31 1.874549249    testwifi.here         vulture.lan           DNS
158    XXX
     32 1.900313841    testwifi.here         vulture.lan           DNS
106    XXX
     33 1.900576751    vulture.lan           api.github.com        TCP
74     43292 → https(443) [SYN] Seq=0 Win=29200 Len=0 MSS=1460 SACK_PERM=1
TSval=4190850269 TSecr=0 WS=128
     34 1.903210022    vulture.lan           testwifi.here         DNS
88     XXX
     35 1.903272421    vulture.lan           testwifi.here         DNS
87     XXX
     36 1.903318706    vulture.lan           testwifi.here         DNS
86     XXX
     37 1.903361506    vulture.lan           testwifi.here         DNS
85     XXX
     38 1.906623630    testwifi.here         vulture.lan           DNS
137    XXX
     39 1.906956508    testwifi.here         vulture.lan           DNS
137    XXX
     40 1.907649896    testwifi.here         vulture.lan           DNS
87     XXX
     41 1.908724609    testwifi.here         vulture.lan           DNS
156    XXX
     42 1.925632754    api.github.com        vulture.lan           TCP
74     https(443) → 43292 [SYN, ACK] Seq=0 Ack=1 Win=28480 Len=0 MSS=1436
SACK_PERM=1 TSval=3140518241 TSecr=4190850269 WS=1024
     43 1.925691567    vulture.lan           api.github.com        TCP
66     43292 → https(443) [ACK] Seq=1 Ack=1 Win=29312 Len=0
TSval=4190850294 TSecr=3140518241
     44 1.926622012    vulture.lan           api.github.com        HTTP
 308    GET /users/charignon/repos HTTP/1.1
     45 1.950026748    api.github.com        vulture.lan           TCP
66     https(443) → 43292 [FIN, ACK] Seq=1 Ack=243 Win=29696 Len=0
TSval=3140518247 TSecr=4190850295
     46 1.950219585    vulture.lan           api.github.com        TCP
66     43292 → https(443) [FIN, ACK] Seq=243 Ack=2 Win=29312 Len=0
TSval=4190850318 TSecr=3140518247
     47 1.972892477    api.github.com        vulture.lan           TCP
66     https(443) → 43292 [ACK] Seq=2 Ack=244 Win=29696 Len=0
TSval=3140518253 TSecr=4190850318
```

vulture.lan is my computer.
testwifi.here is my DNS.
starting at packet 33 we talk to a second github server, not like the one
we talk to in packet 13.
I configured wireshark to inspect encrypted traffic to see the HTTP
request in there.
What you can notice is that in the synchronous (failing) case, packet 28
is resetting (unexpectedly as far as I can tell) the TCP connection, and
we attempt a retry, which does not work.

And here is the trace for the `url-retrieve` case, where everything goes
well:

```
No.     Time           Source                Destination           Protocol
Length Info
      1 0.000000000    vulture.lan           testwifi.here         DNS
74     XXX
      2 0.000068027    vulture.lan           testwifi.here         DNS
74     XXX
      3 0.008919887    testwifi.here         vulture.lan           DNS
158    XXX
      4 0.035184884    testwifi.here         vulture.lan           DNS
106    XXX
      5 0.107029734    vulture.lan           api.github.com        TCP
74     55428 → https(443) [SYN] Seq=0 Win=29200 Len=0 MSS=1460 SACK_PERM=1
TSval=2499323470 TSecr=0 WS=128
      6 0.134137564    api.github.com        vulture.lan           TCP
74     https(443) → 55428 [SYN, ACK] Seq=0 Ack=1 Win=28480 Len=0 MSS=1436
SACK_PERM=1 TSval=2535678566 TSecr=2499323470 WS=1024
      7 0.134221939    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=1 Ack=1 Win=29312 Len=0
TSval=2499323498 TSecr=2535678566
      8 0.134313037    vulture.lan           api.github.com        TLSv1.3
583    Client Hello
      9 0.162958206    api.github.com        vulture.lan           TLSv1.3
1490   Server Hello, Change Cipher Spec, Encrypted Extensions
     10 0.163005821    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=518 Ack=1425 Win=32128 Len=0
TSval=2499323526 TSecr=2535678573
     11 0.163752256    api.github.com        vulture.lan           TCP
1490   https(443) → 55428 [ACK] Seq=1425 Ack=518 Win=29696 Len=1424
TSval=2535678573 TSecr=2499323498 [TCP segment of a reassembled PDU]
     12 0.163769338    vulture.lan           api.github.com        TLSv1.3
72     Change Cipher Spec
     13 0.164938244    api.github.com        vulture.lan           TLSv1.3
615    Certificate, Certificate Verify, Finished
     14 0.164970615    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=524 Ack=3398 Win=37760 Len=0
TSval=2499323528 TSecr=2535678573
     15 0.229427678    api.github.com        vulture.lan           TCP
66     https(443) → 55428 [ACK] Seq=3398 Ack=524 Win=29696 Len=0
TSval=2535678590 TSecr=2499323527
     16 0.229496471    vulture.lan           api.github.com        HTTP
 388    GET /users/charignon/repos HTTP/1.1
     17 0.253820590    api.github.com        vulture.lan           TCP
66     https(443) → 55428 [ACK] Seq=3398 Ack=846 Win=30720 Len=0
TSval=2535678596 TSecr=2499323593
     18 0.255187068    api.github.com        vulture.lan           TLSv1.3
145    New Session Ticket
     19 0.256462285    api.github.com        vulture.lan           TLSv1.3
145    New Session Ticket
     20 0.256590338    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=846 Ack=3556 Win=37760 Len=0
TSval=2499323620 TSecr=2535678596
     21 0.485155486    api.github.com        vulture.lan           TLSv1.3
1458   [SSL segment of a reassembled PDU]
     22 0.485523530    api.github.com        vulture.lan           TLSv1.3
1458   [SSL segment of a reassembled PDU]
     23 0.485567467    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=846 Ack=6340 Win=43392 Len=0
TSval=2499323849 TSecr=2535678654
     24 0.486210922    api.github.com        vulture.lan           TLSv1.3
1490   [SSL segment of a reassembled PDU]
     25 0.486551493    api.github.com        vulture.lan           TLSv1.3
1276   [SSL segment of a reassembled PDU]
     26 0.486588968    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=846 Ack=8974 Win=49152 Len=0
TSval=2499323850 TSecr=2535678654
     27 0.487345701    api.github.com        vulture.lan           TLSv1.3
1490   [SSL segment of a reassembled PDU]
     28 0.487710595    api.github.com        vulture.lan           TLSv1.3
1490   [SSL segment of a reassembled PDU] [TCP segment of a reassembled PDU]
     29 0.487741685    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=846 Ack=11822 Win=54784 Len=0
TSval=2499323851 TSecr=2535678654
     30 0.489279277    api.github.com        vulture.lan           TLSv1.3
1490   [SSL segment of a reassembled PDU] [TCP segment of a reassembled PDU]
     31 0.490560244    api.github.com        vulture.lan           TLSv1.3
1490   [SSL segment of a reassembled PDU] [TCP segment of a reassembled PDU]
     32 0.490600325    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=846 Ack=14670 Win=60544 Len=0
TSval=2499323854 TSecr=2535678654
     33 0.491903018    api.github.com        vulture.lan           TLSv1.3
1490   [SSL segment of a reassembled PDU] [TCP segment of a reassembled PDU]
     34 0.493248621    api.github.com        vulture.lan           TLSv1.3
1490   [SSL segment of a reassembled PDU] [TCP segment of a reassembled PDU]
     35 0.493286446    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=846 Ack=17518 Win=66176 Len=0
TSval=2499323857 TSecr=2535678654
     36 0.501270566    vulture.lan           testwifi.here         DNS
86     XXX
     37 0.501341962    vulture.lan           testwifi.here         DNS
85     XXX
     38 0.503268399    testwifi.here         vulture.lan           DNS
137    XXX
     39 0.504365465    testwifi.here         vulture.lan           DNS
137    XXX
     40 0.507593395    api.github.com        vulture.lan           HTTP
 75     HTTP/1.1 200 OK  (application/json)
     41 0.549680721    vulture.lan           api.github.com        TCP
66     55428 → https(443) [ACK] Seq=846 Ack=17527 Win=66176 Len=0
TSval=2499323913 TSecr=2535678660
```

5) I noticed that adding `(sit-for 0.5)` in various places throughout the
codepath would consistently make the bug go away. For example in
`open-network-stream` after opening the stream, or at the beginning of
`url-http-create-request`.

6) I couldn't find information online indicating that github changed
anything in their network request handling that broke other tools.

I am using Emacs 26.1:

In GNU Emacs 26.1 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.22.30)
 of 2018-07-05 built on juergen
Windowing system distributor 'The X.Org Foundation', version 11.0.12003000
System Description: Manjaro Linux

[-- Attachment #2: Type: text/html, Size: 19507 bytes --]

bug#34341: Seems fixed in latest master 5259fdf96073febd18e83785960c443bdf02a310

[Laurent Charignon]

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

I built the latest master I could find in git
(5259fdf96073febd18e83785960c443bdf02a31) and the bug seems fixed there!

From bisecting, I know that the fix is in between 191a5568db (working) and
2a3bd6798e96708 (not working). It is hard to pinpoint further due to
segfaults that were introduced then fixed in this range. My best guess is
that "Fix unlikely races with GnuTLS, datagrams" (e87e6a24c4) resolved this.

[-- Attachment #2: Type: text/html, Size: 612 bytes --]

bug#34341: more observations

[Mikhail Rudenko]

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

I've also hit this bug, here's a couple of observations:

(1) Looks like the version of gnutls affects the behavior of
url-retrieve-synchronously.
My conclusion is that the bug manifests with gnutls >= 3.6. I confirmed
this on Arch with
emacs 26.1 and on Ubuntu bionic/cosmic with emacs 25.2

(2) Weird workaround, which works for me: add (setq gnutls-log-level 1) to
the end of init.el

[-- Attachment #2: Type: text/html, Size: 510 bytes --]

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Noam Postavsky]

Laurent Charignon <l.charignon@gmail.com> writes:

> #### Context
>
> The symptom of this issue first came up in the context of `ghub`, a package
> that offers interfacing with Github from emacs (issue:
> https://github.com/magit/ghub/issues/81).
>
> Multiple users of the `ghub` package started observing that
> synchronously retrieving content from the github API through ghub
> returned blank responses.

> (switch-to-buffer (url-retrieve-synchronously "
> https://api.github.com/users/charignon/repos")
>                   (buffer-string))

> From bisecting, I know that the fix is in between 191a5568db (working) and
> 2a3bd6798e96708 (not working). It is hard to pinpoint further due to
> segfaults that were introduced then fixed in this range. My best guess is
> that "Fix unlikely races with GnuTLS, datagrams" (e87e6a24c4) resolved this.

I can confirm that cherrypicking this commit fixes the problem on my
Arch box which has gnutls 3.6.7 (doesn't occur at all on my Debian box
with gnutls 3.5.8).

Since the only recourse for users hitting is rebuilding --without-gnutls
or (advice-add 'gnutls-available-p :override #'ignore) which falls back
to the obsolete tls.el, I'd say this is important enough to backport to
emacs-26.  I'll do that in a couple of days if there are no objections.

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Robert Pluim]

>>>>> On Sat, 04 May 2019 11:18:30 -0400, Noam Postavsky <npostavs@gmail.com> said:

    Noam> Laurent Charignon <l.charignon@gmail.com> writes:
    >> #### Context
    >> 
    >> The symptom of this issue first came up in the context of
    >> `ghub`, a package that offers interfacing with Github from
    >> emacs (issue: https://github.com/magit/ghub/issues/81).
    >> 
    >> Multiple users of the `ghub` package started observing that
    >> synchronously retrieving content from the github API through
    >> ghub returned blank responses.

    >> (switch-to-buffer (url-retrieve-synchronously "
    >> https://api.github.com/users/charignon/repos") (buffer-string))

    >> From bisecting, I know that the fix is in between 191a5568db
    >> (working) and 2a3bd6798e96708 (not working). It is hard to
    >> pinpoint further due to segfaults that were introduced then
    >> fixed in this range. My best guess is that "Fix unlikely races
    >> with GnuTLS, datagrams" (e87e6a24c4) resolved this.

    Noam> I can confirm that cherrypicking this commit fixes the
    Noam> problem on my Arch box which has gnutls 3.6.7 (doesn't occur
    Noam> at all on my Debian box with gnutls 3.5.8).

    Noam> Since the only recourse for users hitting is rebuilding
    Noam> --without-gnutls or (advice-add 'gnutls-available-p
    Noam> :override #'ignore) which falls back to the obsolete tls.el,
    Noam> I'd say this is important enough to backport to emacs-26.
    Noam> I'll do that in a couple of days if there are no objections.

Are you sure thatʼs the only recourse? When I hit this issue, turning
off TLS1.3 support helped as well. (see
<https://lists.gnu.org/archive/html/emacs-devel/2019-02/msg00393.html>
and
<https://lists.gnu.org/archive/html/help-gnu-emacs/2019-02/msg00196.html>)

Something like:

(setq gnutls-algorithm-priority "NORMAL:-VERS-TLS1.3")

would be one way to turn off TLS1.3

Robert

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Noam Postavsky]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Sat, 04 May 2019 11:18:30 -0400, Noam Postavsky <npostavs@gmail.com> said:
>
>     Noam> I can confirm that cherrypicking this commit fixes the
>     Noam> problem on my Arch box which has gnutls 3.6.7 (doesn't occur
>     Noam> at all on my Debian box with gnutls 3.5.8).
>
>     Noam> Since the only recourse for users hitting is rebuilding
>     Noam> --without-gnutls or (advice-add 'gnutls-available-p
>     Noam> :override #'ignore) which falls back to the obsolete tls.el,
>     Noam> I'd say this is important enough to backport to emacs-26.
>     Noam> I'll do that in a couple of days if there are no objections.
>
> Are you sure thatʼs the only recourse? When I hit this issue, turning
> off TLS1.3 support helped as well. (see
> <https://lists.gnu.org/archive/html/emacs-devel/2019-02/msg00393.html>
> and
> <https://lists.gnu.org/archive/html/help-gnu-emacs/2019-02/msg00196.html>)
>
> Something like:
>
> (setq gnutls-algorithm-priority "NORMAL:-VERS-TLS1.3")
>
> would be one way to turn off TLS1.3

Thanks, I wasn't aware of this, I'll check if that helps later today.  I
think it makes sense to backport regardless though.

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Robert Pluim]

>>>>> On Mon, 06 May 2019 08:10:32 -0400, Noam Postavsky <npostavs@gmail.com> said:

    Noam> Thanks, I wasn't aware of this, I'll check if that helps
    Noam> later today.  I think it makes sense to backport regardless
    Noam> though.

I do too, but last time Eli wasn't too enthusiastic about it.

Robert

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Mon, 06 May 2019 14:15:27 +0200
> Cc: Laurent Charignon <l.charignon@gmail.com>, 34341@debbugs.gnu.org
> 
> >>>>> On Mon, 06 May 2019 08:10:32 -0400, Noam Postavsky <npostavs@gmail.com> said:
> 
>     Noam> Thanks, I wasn't aware of this, I'll check if that helps
>     Noam> later today.  I think it makes sense to backport regardless
>     Noam> though.
> 
> I do too, but last time Eli wasn't too enthusiastic about it.

It's a complex change, and we don't even know whether emacs-26 will be
used for another "regular" release.  What if we need it for some
emergency release, for example?  Having to remember there was this
cherry-pick, and branching from before it, reverting it, is not my
idea of fun.

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Noam Postavsky]

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

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Robert Pluim <rpluim@gmail.com>
>> Date: Mon, 06 May 2019 14:15:27 +0200
>> Cc: Laurent Charignon <l.charignon@gmail.com>, 34341@debbugs.gnu.org
>> 
>> >>>>> On Mon, 06 May 2019 08:10:32 -0400, Noam Postavsky <npostavs@gmail.com> said:
>> 
>>     Noam> Thanks, I wasn't aware of this, I'll check if that helps
>>     Noam> later today.  I think it makes sense to backport regardless
>>     Noam> though.

I can confirm that

    (setq gnutls-algorithm-priority "NORMAL:-VERS-TLS1.3") 

avoids the problem on my Arch box (has gnutls v3.6.7).

>> I do too, but last time Eli wasn't too enthusiastic about it.
>
> It's a complex change, and we don't even know whether emacs-26 will be
> used for another "regular" release.  What if we need it for some
> emergency release, for example?  Having to remember there was this
> cherry-pick, and branching from before it, reverting it, is not my
> idea of fun.

I found this smaller change fixes it also.  Is it simple enough to for
emacs-26?  Otherwise, we should set gnutls-algorithm-priority because
(as far as I understand) we have a broken TLS1.3 implementation at the
momement.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch --]
[-- Type: text/x-diff, Size: 1176 bytes --]

From 6d5f5670443f7adfbd6993173ad997ce25827775 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 6 May 2019 19:55:17 -0400
Subject: [PATCH] Handle GNUTLS_E_AGAIN in emacs_gnutls_read (Bug#34341)

* src/gnutls.c (emacs_gnutls_read): Similar to emacs_gnutls_write,
when gnutls_record_recv returns GNUTLS_E_AGAIN set errno to EGAIN.
---
 src/gnutls.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index 3c16b6c9c3..b724c3592d 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -753,8 +753,15 @@ emacs_gnutls_read (struct Lisp_Process *proc, char *buf, ptrdiff_t nbyte)
     /* The peer closed the connection. */
     return 0;
   else if (emacs_gnutls_handle_error (state, rtnval))
-    /* non-fatal error */
-    return -1;
+    {
+      /* If we get GNUTLS_E_AGAIN, then set errno appropriately so that
+         wait_reading_process_output retries the correct way instead of
+         erroring out.  */
+      if (rtnval == GNUTLS_E_AGAIN)
+        errno = EAGAIN;
+      /* non-fatal error */
+      return -1;
+    }
   else {
     /* a fatal error occurred */
     return 0;
-- 
2.11.0

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Eli Zaretskii]

> From: Noam Postavsky <npostavs@gmail.com>
> Cc: Robert Pluim <rpluim@gmail.com>,  34341@debbugs.gnu.org,  l.charignon@gmail.com
> Date: Mon, 06 May 2019 20:16:52 -0400
> 
> I found this smaller change fixes it also.  Is it simple enough to for
> emacs-26?

Yes, if we are sure it fixes the problem.

Thanks.

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Robert Pluim]

>>>>> On Mon, 06 May 2019 20:16:52 -0400, Noam Postavsky <npostavs@gmail.com> said:

    Noam> Eli Zaretskii <eliz@gnu.org> writes:
    >>> From: Robert Pluim <rpluim@gmail.com> Date: Mon, 06 May 2019
    >>> 14:15:27 +0200 Cc: Laurent Charignon <l.charignon@gmail.com>,
    >>> 34341@debbugs.gnu.org
    >>> 
    >>> >>>>> On Mon, 06 May 2019 08:10:32 -0400, Noam Postavsky
    >>> <npostavs@gmail.com> said:
    >>> 
    Noam> Thanks, I wasn't aware of this, I'll check if that helps
    Noam> later today.  I think it makes sense to backport regardless
    Noam> though.

    Noam> I can confirm that

    Noam>     (setq gnutls-algorithm-priority "NORMAL:-VERS-TLS1.3")

    Noam> avoids the problem on my Arch box (has gnutls v3.6.7).

OK, so itʼs the same issue.

    Noam> I found this smaller change fixes it also.  Is it simple
    Noam> enough to for emacs-26?  Otherwise, we should set
    Noam> gnutls-algorithm-priority because (as far as I understand)
    Noam> we have a broken TLS1.3 implementation at the momement.

If we do decide to disable TLS1.3, which Iʼm not really in favour of,
changing the defaults in lisp/net/gnutls.el:gnutls-boot-parameters
might be better than setting gnutls-algorithm-priority. Iʼd prefer
your gnutls.c patch, although I haven't had a chance to test it yet.

Robert

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Robert Pluim]

>>>>> On Tue, 07 May 2019 08:35:00 +0200, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Mon, 06 May 2019 20:16:52 -0400, Noam Postavsky <npostavs@gmail.com> said:
    Noam> I found this smaller change fixes it also.  Is it simple
    Noam> enough to for emacs-26?  Otherwise, we should set
    Noam> gnutls-algorithm-priority because (as far as I understand)
    Noam> we have a broken TLS1.3 implementation at the momement.

    Robert> If we do decide to disable TLS1.3, which Iʼm not really in
    Robert> favour of, changing the defaults in
    Robert> lisp/net/gnutls.el:gnutls-boot-parameters might be better
    Robert> than setting gnutls-algorithm-priority. Iʼd prefer your
    Robert> gnutls.c patch, although I haven't had a chance to test it
    Robert> yet.

That patch to src/gnutls.c fixes my testcase, which is imap over
TLS1.3 to gmail. Itʼs pretty simple, I think it could go to
emacs-26. Eli?

Robert

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Date: Sun, 12 May 2019 15:01:03 +0200
> Cc: l.charignon@gmail.com, 34341@debbugs.gnu.org
> 
> >>>>> On Mon, 06 May 2019 20:16:52 -0400, Noam Postavsky <npostavs@gmail.com> said:
>     Noam> I found this smaller change fixes it also.  Is it simple
>     Noam> enough to for emacs-26?  Otherwise, we should set
>     Noam> gnutls-algorithm-priority because (as far as I understand)
>     Noam> we have a broken TLS1.3 implementation at the momement.
> 
>     Robert> If we do decide to disable TLS1.3, which Iʼm not really in
>     Robert> favour of, changing the defaults in
>     Robert> lisp/net/gnutls.el:gnutls-boot-parameters might be better
>     Robert> than setting gnutls-algorithm-priority. Iʼd prefer your
>     Robert> gnutls.c patch, although I haven't had a chance to test it
>     Robert> yet.
> 
> That patch to src/gnutls.c fixes my testcase, which is imap over
> TLS1.3 to gmail. Itʼs pretty simple, I think it could go to
> emacs-26. Eli?

Didn't I already answer that?  If not, I guess you are now talking
about some patch which I didn't see, so please show it.

Thanks.

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Robert Pluim]

>>>>> On Sun, 12 May 2019 17:02:56 +0300, Eli Zaretskii <eliz@gnu.org> said:
    >> That patch to src/gnutls.c fixes my testcase, which is imap
    >> over TLS1.3 to gmail. Itʼs pretty simple, I think it could go
    >> to emacs-26. Eli?

    Eli> Didn't I already answer that?  If not, I guess you are now
    Eli> talking about some patch which I didn't see, so please show
    Eli> it.

From 6d5f5670443f7adfbd6993173ad997ce25827775 Mon Sep 17 00:00:00 2001
From: Noam Postavsky <npostavs@gmail.com>
Date: Mon, 6 May 2019 19:55:17 -0400
Subject: [PATCH] Handle GNUTLS_E_AGAIN in emacs_gnutls_read (Bug#34341)

* src/gnutls.c (emacs_gnutls_read): Similar to emacs_gnutls_write,
when gnutls_record_recv returns GNUTLS_E_AGAIN set errno to EGAIN.
---
 src/gnutls.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/gnutls.c b/src/gnutls.c
index 3c16b6c9c3..b724c3592d 100644
--- a/src/gnutls.c
+++ b/src/gnutls.c
@@ -753,8 +753,15 @@ emacs_gnutls_read (struct Lisp_Process *proc, char *buf, ptrdiff_t nbyte)
     /* The peer closed the connection. */
     return 0;
   else if (emacs_gnutls_handle_error (state, rtnval))
-    /* non-fatal error */
-    return -1;
+    {
+      /* If we get GNUTLS_E_AGAIN, then set errno appropriately so that
+         wait_reading_process_output retries the correct way instead of
+         erroring out.  */
+      if (rtnval == GNUTLS_E_AGAIN)
+        errno = EAGAIN;
+      /* non-fatal error */
+      return -1;
+    }
   else {
     /* a fatal error occurred */
     return 0;
-- 
2.11.0

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: npostavs@gmail.com,  l.charignon@gmail.com,  34341@debbugs.gnu.org
> Date: Sun, 12 May 2019 16:55:21 +0200
> 
> >>>>> On Sun, 12 May 2019 17:02:56 +0300, Eli Zaretskii <eliz@gnu.org> said:
>     >> That patch to src/gnutls.c fixes my testcase, which is imap
>     >> over TLS1.3 to gmail. Itʼs pretty simple, I think it could go
>     >> to emacs-26. Eli?
> 
>     Eli> Didn't I already answer that?  If not, I guess you are now
>     Eli> talking about some patch which I didn't see, so please show
>     Eli> it.
> 
> >From 6d5f5670443f7adfbd6993173ad997ce25827775 Mon Sep 17 00:00:00 2001
> From: Noam Postavsky <npostavs@gmail.com>
> Date: Mon, 6 May 2019 19:55:17 -0400
> Subject: [PATCH] Handle GNUTLS_E_AGAIN in emacs_gnutls_read (Bug#34341)
> 
> * src/gnutls.c (emacs_gnutls_read): Similar to emacs_gnutls_write,
> when gnutls_record_recv returns GNUTLS_E_AGAIN set errno to EGAIN.

I definitely said it was okay to push that one to emacs-26:

  https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34341#34

Thanks.

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Robert Pluim]

>>>>> On Sun, 12 May 2019 18:23:19 +0300, Eli Zaretskii <eliz@gnu.org> said:

    >> From: Robert Pluim <rpluim@gmail.com> Cc: npostavs@gmail.com,
    >> l.charignon@gmail.com, 34341@debbugs.gnu.org Date: Sun, 12 May
    >> 2019 16:55:21 +0200
    >> 
    >> >>>>> On Sun, 12 May 2019 17:02:56 +0300, Eli Zaretskii
    >> <eliz@gnu.org> said: >> That patch to src/gnutls.c fixes my
    >> testcase, which is imap >> over TLS1.3 to gmail. Itʼs pretty
    >> simple, I think it could go >> to emacs-26. Eli?
    >> 
    Eli> Didn't I already answer that?  If not, I guess you are now
    Eli> talking about some patch which I didn't see, so please show
    Eli> it.
    >> 
    >> >From 6d5f5670443f7adfbd6993173ad997ce25827775 Mon Sep 17
    >> 00:00:00 2001 From: Noam Postavsky <npostavs@gmail.com> Date:
    >> Mon, 6 May 2019 19:55:17 -0400 Subject: [PATCH] Handle
    >> GNUTLS_E_AGAIN in emacs_gnutls_read (Bug#34341)
    >> 
    >> * src/gnutls.c (emacs_gnutls_read): Similar to
    >> emacs_gnutls_write, when gnutls_record_recv returns
    >> GNUTLS_E_AGAIN set errno to EGAIN.

    Eli> I definitely said it was okay to push that one to emacs-26:

    Eli>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34341#34

So you did, I missed that one.

Robert

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Eli Zaretskii]

> From: Robert Pluim <rpluim@gmail.com>
> Cc: npostavs@gmail.com,  34341@debbugs.gnu.org,  l.charignon@gmail.com
> Date: Sun, 12 May 2019 17:39:28 +0200
> 
> 
>     Eli> I definitely said it was okay to push that one to emacs-26:
> 
>     Eli>   https://debbugs.gnu.org/cgi/bugreport.cgi?bug=34341#34
> 
> So you did, I missed that one.

No sweat.

bug#34341: 26.1; url-retrieve-synchronously returns blank buffer

[Noam Postavsky]

tags 34341 fixed
close 34341 26.3
quit

Robert Pluim <rpluim@gmail.com> writes:

>     Noam> I found this smaller change fixes it also.  Is it simple
>     Noam> enough to for emacs-26?  Otherwise, we should set
>     Noam> gnutls-algorithm-priority because (as far as I understand)
>     Noam> we have a broken TLS1.3 implementation at the momement.
>
> That patch to src/gnutls.c fixes my testcase, which is imap over
> TLS1.3 to gmail.

Okay, good, it's not just me then. :)

I think this patch is going to get more important over time, as more
people upgrade gnutls versions, and more servers start supporting
TLS1.3.

Pushed to emacs-26.

202ff53da2 2019-05-13T20:15:31-04:00 "Handle GNUTLS_E_AGAIN in emacs_gnutls_read (Bug#34341)"
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=202ff53da267f9fa15f438e9c38603bbead6e890