| Message ID | 20220801090749.11655-1-ludo@gnu.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#56867] download: Do not wrap TLS port on GnuTLS >= 3.7.7. | expand |
Some objections on error handling (I don't know much about the wrapping) On 01-08-2022 11:07, Ludovic Courtès wrote: > [...] > Hello! > > I'll land a similar change in Guile's (web client) module afterwards > if there are no objections. > > Ludo'. > > diff --git a/guix/build/download.scm b/guix/build/download.scm > index 41583e8143..de094890b3 100644 > --- a/guix/build/download.scm > +++ b/guix/build/download.scm > @@ -245,6 +245,54 @@ (define (print-tls-certificate-error port key args default-printer) > (set-exception-printer! 'tls-certificate-error > print-tls-certificate-error) > > +(define (wrap-record-port-for-gnutls<3.7.7 record port) > + "Return a port that wraps RECORD to ensure that closing it also closes PORT, > +the actual socket port, and its file descriptor. Make sure it does not > +introduce extra buffering (custom ports are buffered by default as of Guile > +3.0.5). > + > +This wrapper is unnecessary with GnuTLS >= 3.7.7, which can automatically > +close SESSION's file descriptor when RECORD is closed." > + (define (read! bv start count) > + (define read > + (catch 'gnutls-error > + (lambda () > + (get-bytevector-n! record bv start count)) > + (lambda (key err proc . rest) > + ;; When responding to "Connection: close" requests, some servers > + ;; close the connection abruptly after sending the response body, > + ;; without doing a proper TLS connection termination. Treat it as > + ;; EOF. This is fixed in GnuTLS 3.7.7. > + (if (eq? err error/premature-termination) > + the-eof-object > + (apply throw key err proc rest))))) Objection: 'catch' makes the backtrace part happening inside the 'get-bytevector-n!' disappear, because it is unwinding, as has been noted a few times (in different contexts) by Attila Lendvai and me. Maybe use 'guard' with an appropriate condition instead? > + (if (module-defined? (resolve-interface '(gnutls)) > + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 resolve-module (and presumably also sets #:ensure #t by default, which sometimes causes 'module not found' messages to be replaced by 'unbound variable', which I don't think is useful behaviour, can #:ensure be set to #false? Greetings, Maxime
Hi, Maxime Devos <maximedevos@telenet.be> skribis: > On 01-08-2022 11:07, Ludovic Courtès wrote: [...] >> + (define (read! bv start count) >> + (define read >> + (catch 'gnutls-error >> + (lambda () >> + (get-bytevector-n! record bv start count)) >> + (lambda (key err proc . rest) >> + ;; When responding to "Connection: close" requests, some servers >> + ;; close the connection abruptly after sending the response body, >> + ;; without doing a proper TLS connection termination. Treat it as >> + ;; EOF. This is fixed in GnuTLS 3.7.7. >> + (if (eq? err error/premature-termination) >> + the-eof-object >> + (apply throw key err proc rest))))) > > Objection: 'catch' makes the backtrace part happening inside the > 'get-bytevector-n!' disappear, because it is unwinding, as has been > noted a few times (in different contexts) by Attila Lendvai and me. > Maybe use 'guard' with an appropriate condition instead? This code was already there and has just been moved around. (It’s also code that will no longer be used going forward.) >> + (if (module-defined? (resolve-interface '(gnutls)) >> + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 > > resolve-module (and presumably also sets #:ensure #t by default, which > sometimes causes 'module not found' messages to be replaced by > 'unbound variable', which I don't think is useful behaviour, can > #:ensure be set to #false? This is unnecessary: see the ‘load-gnutls’ mechanism there. The idiom above is already used in a couple of places. Thanks for your feedback! Ludo’.
Ludovic Courtès <ludo@gnu.org> skribis: > The custom input/output port wrapping the TLS session record port would > introduce overhead, and it would also prevent its uses in a non-blocking > context--e.g., with Fibers. The port close mechanism added in GnuTLS > 3.7.7 allows us to get rid of that wrapper. > > * guix/build/download.scm (wrap-record-port-for-gnutls<3.7.7): New > procedure, with code formerly in 'tls-wrap'. > (tls-wrap): Check for 'set-session-record-port-close!' and use it when > available; otherwise call 'wrap-record-port-for-gnutls<3.7.7'. > --- > guix/build/download.scm | 102 +++++++++++++++++++++------------------- > 1 file changed, 54 insertions(+), 48 deletions(-) Pushed as Guix commit dd573ceea73295c7a872088ecd91e5f0fd74bf2b. Ludo’.
On 02-08-2022 09:59, Ludovic Courtès wrote: >>> + (if (module-defined? (resolve-interface '(gnutls)) >>> + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 >> resolve-module (and presumably also sets #:ensure #t by default, which >> sometimes causes 'module not found' messages to be replaced by >> 'unbound variable', which I don't think is useful behaviour, can >> #:ensure be set to #false? > This is unnecessary: see the ‘load-gnutls’ mechanism there. The idiom > above is already used in a couple of places. I have looked at the 'load-gnutls' procedure, but I do not see how it avoids the issue I mentioned (*). I have also seen this idiom (resolve-interface and friends with #:ensure #t) before, in other places, but that doesn't make the idiom correct -- in fact, _because_ I've seen the idiom elsewhere causing problems, I recommend avoiding the same mistake here (and preferably also eliminating it elsewhere). More generally, the second sentence is a logical fallacy, a variant of "ad populum" -- the prevalency of a mistake does not make it correct and does not invalidate evidence of it being a mistake. To be clear, I am not referring to the existence/absence of compilation errors when compiling the Guix package without gnutls in the build environment, but to the confusing _contents_ of the error message and the odd semantics of #:ensure #t, and not only at compilation time but also at runtime. (*) The autoloading of gnutls in load-gnutls avoids compilation errors when gnutls is absent, but by the way it does it, it causes the module to be registered as 'it exists' even when it doesn't, so the information in the module system of Guix becomes incorrect. Greetings, Maxime.
Hi, Maxime Devos <maximedevos@telenet.be> skribis: > On 02-08-2022 09:59, Ludovic Courtès wrote: >>>> + (if (module-defined? (resolve-interface '(gnutls)) >>>> + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 >>> resolve-module (and presumably also sets #:ensure #t by default, which >>> sometimes causes 'module not found' messages to be replaced by >>> 'unbound variable', which I don't think is useful behaviour, can >>> #:ensure be set to #false? >> This is unnecessary: see the ‘load-gnutls’ mechanism there. The idiom >> above is already used in a couple of places. > > I have looked at the 'load-gnutls' procedure, but I do not see how it > avoids the issue I mentioned (*). [...] > (*) The autoloading of gnutls in load-gnutls avoids compilation errors > when gnutls is absent, but by the way it does it, it causes the module > to be registered as 'it exists' even when it doesn't, so the > information in the module system of Guix becomes incorrect. I understand what you’re saying (I’m quite familiar with Guile’s module system :-) and I do agree that #:ensure #t can lead to bad surprises), but I don’t think this is correct: --8<---------------cut here---------------start------------->8--- scheme@(guile-user)> (resolve-interface '(xxx)) ice-9/boot-9.scm:1685:16: In procedure raise-exception: no code for module (xxx) Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue. scheme@(guile-user) [1]> ,q scheme@(guile-user)> (resolve-module '(xxx) #f #:ensure #f) $1 = #f --8<---------------cut here---------------end--------------->8--- This is because ‘resolve-interface’ does (resolve-module … #:ensure #f). Does that make sense? Thanks, Ludo’.
On 05-08-2022 10:31, Ludovic Courtès wrote: > I understand what you’re saying (I’m quite familiar with Guile’s module > system :-) and I do agree that #:ensure #t can lead to bad surprises), > but I don’t think this is correct: > > --8<---------------cut here---------------start------------->8--- > scheme@(guile-user)> (resolve-interface '(xxx)) > ice-9/boot-9.scm:1685:16: In procedure raise-exception: > no code for module (xxx) > > Entering a new prompt. Type `,bt' for a backtrace or `,q' to continue. > scheme@(guile-user) [1]> ,q > scheme@(guile-user)> (resolve-module '(xxx) #f #:ensure #f) > $1 = #f > --8<---------------cut here---------------end--------------->8--- > > This is because ‘resolve-interface’ does (resolve-module … #:ensure #f). > > Does that make sense? Oops, I thought the #:ensure #f was universal to all the resolve-... interfaces, but apparently not for resole-interface! In that case, no problem, though I'd like to eventually make some changes to the Guile docs for clarity (and maybe change the default #:ensure #t -> #f) Greetings, Maxime.
diff --git a/guix/build/download.scm b/guix/build/download.scm index 41583e8143..de094890b3 100644 --- a/guix/build/download.scm +++ b/guix/build/download.scm @@ -245,6 +245,54 @@ (define (print-tls-certificate-error port key args default-printer) (set-exception-printer! 'tls-certificate-error print-tls-certificate-error) +(define (wrap-record-port-for-gnutls<3.7.7 record port) + "Return a port that wraps RECORD to ensure that closing it also closes PORT, +the actual socket port, and its file descriptor. Make sure it does not +introduce extra buffering (custom ports are buffered by default as of Guile +3.0.5). + +This wrapper is unnecessary with GnuTLS >= 3.7.7, which can automatically +close SESSION's file descriptor when RECORD is closed." + (define (read! bv start count) + (define read + (catch 'gnutls-error + (lambda () + (get-bytevector-n! record bv start count)) + (lambda (key err proc . rest) + ;; When responding to "Connection: close" requests, some servers + ;; close the connection abruptly after sending the response body, + ;; without doing a proper TLS connection termination. Treat it as + ;; EOF. This is fixed in GnuTLS 3.7.7. + (if (eq? err error/premature-termination) + the-eof-object + (apply throw key err proc rest))))) + + (if (eof-object? read) + 0 + read)) + (define (write! bv start count) + (put-bytevector record bv start count) + (force-output record) + count) + (define (get-position) + (port-position record)) + (define (set-position! new-position) + (set-port-position! record new-position)) + (define (close) + (unless (port-closed? port) + (close-port port)) + (unless (port-closed? record) + (close-port record))) + + (define (unbuffered port) + (setvbuf port 'none) + port) + + (unbuffered + (make-custom-binary-input/output-port "gnutls wrapped port" read! write! + get-position set-position! + close))) + (define* (tls-wrap port server #:key (verify-certificate? #t)) "Return PORT wrapped in a TLS connection to SERVER. SERVER must be a DNS host name without trailing dot." @@ -317,55 +365,13 @@ (define (log level str) (apply throw args)))) (let ((record (session-record-port session))) - (define (read! bv start count) - (define read - (catch 'gnutls-error - (lambda () - (get-bytevector-n! record bv start count)) - (lambda (key err proc . rest) - ;; When responding to "Connection: close" requests, some - ;; servers close the connection abruptly after sending the - ;; response body, without doing a proper TLS connection - ;; termination. Treat it as EOF. - (if (eq? err error/premature-termination) - the-eof-object - (apply throw key err proc rest))))) - - (if (eof-object? read) - 0 - read)) - (define (write! bv start count) - (put-bytevector record bv start count) - (force-output record) - count) - (define (get-position) - (port-position record)) - (define (set-position! new-position) - (set-port-position! record new-position)) - (define (close) - (unless (port-closed? port) - (close-port port)) - (unless (port-closed? record) - (close-port record))) - - (define (unbuffered port) - (setvbuf port 'none) - port) - (setvbuf record 'block) - - ;; Return a port that wraps RECORD to ensure that closing it also - ;; closes PORT, the actual socket port, and its file descriptor. - ;; Make sure it does not introduce extra buffering (custom ports - ;; are buffered by default as of Guile 3.0.5). - ;; XXX: This wrapper would be unnecessary if GnuTLS could - ;; automatically close SESSION's file descriptor when RECORD is - ;; closed, but that doesn't seem to be possible currently (as of - ;; 3.6.9). - (unbuffered - (make-custom-binary-input/output-port "gnutls wrapped port" read! write! - get-position set-position! - close))))) + (if (module-defined? (resolve-interface '(gnutls)) + 'set-session-record-port-close!) ;GnuTLS >= 3.7.7 + (let ((close-wrapped-port (lambda (_) (close-port port)))) + (set-session-record-port-close! record close-wrapped-port) + record) + (wrap-record-port-for-gnutls<3.7.7 record port))))) (define (ensure-uri uri-or-string) ;XXX: copied from (web http) (cond