[PATCH] cache: Catch valid integer for 'last-expiry-cleanup'.

  • Done
  • quality assurance status badge
Details
3 participants
  • Ludovic Courtès
  • Maxime Devos
  • zimoun
Owner
unassigned
Submitted by
zimoun
Severity
normal
Z
Z
zimoun wrote on 27 May 2022 10:25
(address . guix-patches@gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20220527082519.501697-1-zimon.toutoune@gmail.com

* guix/cache.scm (maybe-remove-expired-cache-entries)[last-expiry-date]: Check
if the date is a valid integer.
---
guix/cache.scm | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

Toggle diff (26 lines)
diff --git a/guix/cache.scm b/guix/cache.scm
index 51009809bd..4a74c42afe 100644
--- a/guix/cache.scm
+++ b/guix/cache.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2022 Simon Tournier <zimon.toutoune@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -93,7 +94,9 @@ (define expiry-file
(define last-expiry-date
(catch 'system-error
(lambda ()
- (call-with-input-file expiry-file read))
+ (match (call-with-input-file expiry-file read)
+ ((? integer? date) date)
+ (_ 0)))
(const 0)))
(when (obsolete? last-expiry-date now cleanup-period)

base-commit: 38bf6c7d0cb588e8d4546db7d2e0bae2ec25183d
--
2.36.0
M
M
Maxime Devos wrote on 27 May 2022 11:54
(address . ludo@gnu.org)
9432b1161007faacc0bd0e58da0bf839e2e8ec39.camel@telenet.be
zimoun schreef op vr 27-05-2022 om 10:25 [+0200]:
Toggle quote (7 lines)
>      (catch 'system-error
>        (lambda ()
> -        (call-with-input-file expiry-file read))
> +        (match (call-with-input-file expiry-file read)
> +          ((? integer? date) date)
> +          (_ 0)))

It might be possible to end up wit hsomething more bogus on some file
system, it's possible to end up with something even more boguse (e.g.,
"unterminated-string), which 'read' doesn't understand. I suggest
using 'get-string-all' + 'number->string'.

For completeness, a comment like

;; Handle the 'write' below being interrupted before the write
;; could complete (e.g. with C-c) and handle file system crashes
;; causing empty files or corrupted contents.

and a regression test in tets/cache.scm would be nice.

Also, I'd switch the catch and the (match ...) because 'read' and
integer? shouldn't raise any 'system-error'.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpCfwBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7jJVAQCaikLYy+Q0fz7265EtiZpF0YnO
jfA8iEbv9sQ37hbOiAD+ME5hYi9Wi481pHOf9HcNcP3/5qC9ozWIkkliKc/tgAI=
=4B9U
-----END PGP SIGNATURE-----


Z
Z
zimoun wrote on 27 May 2022 12:28
(name . Maxime Devos)(address . maximedevos@telenet.be)
CAJ3okZ2Rz3dvDC8qYs=DM2cP3ZQg1exHiRmYan-iDQ+0EtGctQ@mail.gmail.com
Hi Maxime,

On Fri, 27 May 2022 at 11:54, Maxime Devos <maximedevos@telenet.be> wrote:
Toggle quote (14 lines)
> zimoun schreef op vr 27-05-2022 om 10:25 [+0200]:

> > (catch 'system-error
> > (lambda ()
> > - (call-with-input-file expiry-file read))
> > + (match (call-with-input-file expiry-file read)
> > + ((? integer? date) date)
> > + (_ 0)))
>
> It might be possible to end up wit hsomething more bogus on some file
> system, it's possible to end up with something even more boguse (e.g.,
> "unterminated-string), which 'read' doesn't understand. I suggest
> using 'get-string-all' + 'number->string'.

I do not see how. The integer is written by Guile using 'write'.
From my understanding, 'read' understands 'write', and vice-versa.


Toggle quote (3 lines)
> Also, I'd switch the catch and the (match ...) because 'read' and
> integer? shouldn't raise any 'system-error'.

All the cases are covered, IMHO.


Cheers,
simon
M
M
Maxime Devos wrote on 27 May 2022 13:12
(name . zimoun)(address . zimon.toutoune@gmail.com)
24883447c6f3d9b27bcc6e11e117ee974916e091.camel@telenet.be
zimoun schreef op vr 27-05-2022 om 12:28 [+0200]:
Toggle quote (10 lines)
> > It might be possible to end up wit hsomething more bogus on some
> > file
> > system, it's possible to end up with something even more boguse
> > (e.g.,
> > "unterminated-string), which 'read' doesn't understand.  I suggest
> > using 'get-string-all' + 'number->string'.
>
> I do not see how.  The integer is written by Guile using 'write'.
> From my understanding, 'read' understands 'write', and vice-versa.

But what if the computer crashes while the file is being made, and due
to file system details, when rebooted, you end up with a non-integer?
E.g. "unterminated-string.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpCyMBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7gT8AQD2twn1+E72CzCF9kLMvtQWTUFo
yITl2m/GN9JUzvttrwD/UDUQcO+I2D/DRY4wySSsxMlAo3n7uXDN6xNo6nO1gwU=
=iTSw
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 27 May 2022 13:17
(name . zimoun)(address . zimon.toutoune@gmail.com)
22ddd5b3d9906b84b0f1ca79dc78964e59345a6d.camel@telenet.be
zimoun schreef op vr 27-05-2022 om 12:28 [+0200]:
Toggle quote (5 lines)
> > Also, I'd switch the catch and the (match ...) because 'read' and
> > integer? shouldn't raise any 'system-error'.
>
> All the cases are covered, IMHO.

Too many cases are covered. E.g., what if the file was a directory?

scheme@(guile-user)> (call-with-input-file "." read)
ice-9/boot-9.scm:1669:16: In procedure raise-exception:
In procedure fport_read: Is een map

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpCzPBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7qPoAQD7aCZXfdItRx4XPRgOOLW8IEZ1
jH20EGIIshZKl3pN+QEAsWJZzPSMkah+eh+hf8iMctpBmMXmMl0IUjn6e+hxyQc=
=0Ksy
-----END PGP SIGNATURE-----


Z
Z
zimoun wrote on 27 May 2022 13:24
(name . Maxime Devos)(address . maximedevos@telenet.be)
CAJ3okZ2EAQLSbTPyQWPcJ4OjuX+qPf6m3N5goGEptEhsSYWV-A@mail.gmail.com
On Fri, 27 May 2022 at 13:17, Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (4 lines)
> scheme@(guile-user)> (call-with-input-file "." read)
> ice-9/boot-9.scm:1669:16: In procedure raise-exception:
> In procedure fport_read: Is een map

Euh, you are overengineering, no? We are talking about an internal
file used by the Guix cache. Yes, if the user tweaks this cache, then
bad things can happen. It is true for almost what lives in
~/.cache/guix.


Cheers,
simon
Z
Z
zimoun wrote on 27 May 2022 13:39
(name . Maxime Devos)(address . maximedevos@telenet.be)
CAJ3okZ1TQTPiNMkei1vYy_GixUtUh+NVsbpjb=qJq-Z7UnDHQA@mail.gmail.com
On Fri, 27 May 2022 at 13:12, Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (4 lines)
> But what if the computer crashes while the file is being made, and due
> to file system details, when rebooted, you end up with a non-integer?
> E.g. "unterminated-string.

If the value returned by 'read' is a non-integer, then it is set to 0
by the 'match'.

Toggle snippet (7 lines)
$ echo -n -e \\x00\\x01\\x02\\x03 > ~/.cache/guix/inferiors/last-expiry-cleanup
$ cat ~/.cache/guix/inferiors/last-expiry-cleanup
\0 [env]
$ ./pre-inst-env guix time-machine --commit=9d795fb -- help
1653651128[env]

The only case is when 'read' fails. Personally, I do not see how it
would be possible. If you have a concrete example, then we can
examine.


Cheers,
simon
M
M
Maxime Devos wrote on 27 May 2022 13:40
(name . zimoun)(address . zimon.toutoune@gmail.com)
0bafbbcf501fc13b9dab096bf03ce7e60afe1c4f.camel@telenet.be
zimoun schreef op vr 27-05-2022 om 13:24 [+0200]:
Toggle quote (11 lines)
> On Fri, 27 May 2022 at 13:17, Maxime Devos <maximedevos@telenet.be> wrote:
>
> > scheme@(guile-user)> (call-with-input-file "." read)
> > ice-9/boot-9.scm:1669:16: In procedure raise-exception:
> > In procedure fport_read: Is een map
>
> Euh, you are overengineering, no? We are talking about an internal
> file used by the Guix cache. Yes, if the user tweaks this cache, then
> bad things can happen. It is true for almost what lives in
> ~/.cache/guix.

Probably yes. Maybe it makes more sense when applied to get-string-all
+ string->number in a limited form:

(or (string->number
(catch 'system-error
(lambda () (call-with-input-file [...] get-string-all))
(lambda arglist
(if (= ENOENT (system-error-errno arglist))
"0" ; file does not exist
(apply throw arglist)))))
0)

Though even then there remain potential problems, try

scheme@(guile-user)> (string->number "#e1e1000")
ice-9/boot-9.scm:1669:16: In procedure raise-exception:
In procedure string->number: Value out of range: 1000

(seems unlikely to encounter such corruption in practice though).

Greetings,
MAxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpC4pxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7kjUAQCJLEmo38K+tLP+UCxxOjaUPHmK
7XpeSjxAW7yPCkl1PgEAq0BTdH3PVeTVIKJJul5JX4gUpCkmuHiv17TeSn0iegc=
=pLaM
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 27 May 2022 13:49
(name . zimoun)(address . zimon.toutoune@gmail.com)
9483424cc3c89e5fa83ccc1e22b1f6cc7d04dd95.camel@telenet.be
zimoun schreef op vr 27-05-2022 om 13:39 [+0200]:
Toggle quote (4 lines)
> The only case is when 'read' fails.  Personally, I do not see how it
> would be possible.  If you have a concrete example, then we can
> examine.

I don't have a 100%-concrete example, but wasn't there some file system
crash mode, where the contents of a new file has not yet been written
to disk yet the length of the file is > 0, so effectively the file
points to an arbitrary range on the disk? E.g., say Guix told the FS
write 1234 to last-expiry-cleanup. Then the FS created last-expiry-
cleanup, choose a range of 4 bytes to save it as, then crashes before
writing the contents. After restarting, the file contains the _old_ 4
bytes.

These old 4 bytes could be the ASCII representation of

"foo

. Then, when 'read' is run (after rebooting), it sees an incomplete
string "foo, so it fails.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpC6tBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7qo5AP9PG6k+5mhppXv1EmyFZqN5Ei4c
vRUZTQ4+luOGOMR4iwEAqw2kaJX02uPQfwDbDDOkWClCugIn9QGRISSnSEvFKAw=
=ejKM
-----END PGP SIGNATURE-----


Z
Z
zimoun wrote on 27 May 2022 14:40
(name . Maxime Devos)(address . maximedevos@telenet.be)
CAJ3okZ2RLDNYS+XzBfaaDEf5_uMW+3dT7OUMfWLV4to_0gsFRQ@mail.gmail.com
Hi Maxime,

On Fri, 27 May 2022 at 13:49, Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (7 lines)
> These old 4 bytes could be the ASCII representation of
>
> "foo
>
> . Then, when 'read' is run (after rebooting), it sees an incomplete
> string "foo, so it fails.

The question is how would 'read' fail or what would 'read' return?
For instance, the patch works for these cases:

- empty file
- non-integer

Now, if you are able to generate an incomplete file (from an integer
or whatever) against the patch fails, then we can examine. However, I
miss what would be the difference between this incomplete file and,
let say, this case:

echo -n -e \\x12 > ~/.cache/guix/inferiors/last-expiry-cleanup

handled by the patch.


Cheers,
simon
M
M
Maxime Devos wrote on 27 May 2022 15:04
(name . zimoun)(address . zimon.toutoune@gmail.com)
02f4e72bba800a2f59064a8bf628357b5a96f1d6.camel@telenet.be
zimoun schreef op vr 27-05-2022 om 14:40 [+0200]:
Toggle quote (24 lines)
> > These old 4 bytes could be the ASCII representation of
> >
> >    "foo
> >
> > .  Then, when 'read' is run (after rebooting), it sees an
> > incomplete
> > string "foo, so it fails.
>
> The question is how would 'read' fail or what would 'read' return?
> For instance, the patch works for these cases:
>
>  - empty file
>  - non-integer
>
> Now, if you are able to generate an incomplete file (from an integer
> or whatever) against the patch fails, then we can examine.  However,
> I
> miss what would be the difference between this incomplete file and,
> let say, this case:
>
>      echo -n -e \\x12 > ~/.cache/guix/inferiors/last-expiry-cleanup
>
> handled by the patch.

The incomplete file is:

"foo

as mentioned previously. Here's how it fails:

scheme@(guile-user)> (call-with-input-file "a" read)
ice-9/boot-9.scm:1669:16: In procedure raise-exception:
In procedure scm_lreadr: a:2:1: end of file in string constant

The difference is that ^R is interpreted as a symbol, whereas "foo
cannot be interpreted as anything at all by 'read'.

Greetings,
Maxime.
"foo
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpDMbxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7uY/AP0YSCfNMQK5NfD6dmr2mNvFQ/1D
TSSH+3utwWSQeKo6FQEA1QbRLfGgqk2X3jSsMyhG3FnjPVROqsl3RDX94wpzcAU=
=rsM8
-----END PGP SIGNATURE-----


Z
Z
zimoun wrote on 27 May 2022 15:23
(name . Maxime Devos)(address . maximedevos@telenet.be)
CAJ3okZ2MWnKreFD_kxPnxaqkTa=LxNG_R1XcGs0EFi6Ly5SQSw@mail.gmail.com
Hi Maxime,

On Fri, 27 May 2022 at 15:04, Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (4 lines)
> scheme@(guile-user)> (call-with-input-file "a" read)
> ice-9/boot-9.scm:1669:16: In procedure raise-exception:
> In procedure scm_lreadr: a:2:1: end of file in string constant

This is an ad-hoc example and not a real test case.

Toggle snippet (8 lines)
$ echo "foo" > ~/.cache/guix/inferiors/last-expiry-cleanup
$ cat ~/.cache/guix/inferiors/last-expiry-cleanup
foo
$ ./pre-inst-env guix time-machine --commit=9d795fb -- help
Usage: guix OPTION | COMMAND ARGS...
Run COMMAND with ARGS, if given.

As previously mentioned, the patch fixes:

- empty file
- non-integer

I am not able to imagine an incomplete file worse than \\x00.


Toggle quote (3 lines)
> The difference is that ^R is interpreted as a symbol, whereas "foo
> cannot be interpreted as anything at all by 'read'.

I do not understand what you mean and I think you are overengineering.

If you are able to produce a corrupted file which breaks "guix
time-machine", then we can examine. Else let move on. :-)


Cheers,
simon
Z
Z
zimoun wrote on 27 May 2022 15:30
(name . Maxime Devos)(address . maximedevos@telenet.be)
CAJ3okZ3YmbYC+XPtY16B_fvh30iWkbh0Quya1Dq0LiwN0=Swqw@mail.gmail.com
On Fri, 27 May 2022 at 15:23, zimoun <zimon.toutoune@gmail.com> wrote:
Toggle quote (8 lines)
> On Fri, 27 May 2022 at 15:04, Maxime Devos <maximedevos@telenet.be> wrote:
>
> > scheme@(guile-user)> (call-with-input-file "a" read)
> > ice-9/boot-9.scm:1669:16: In procedure raise-exception:
> > In procedure scm_lreadr: a:2:1: end of file in string constant
>
> This is an ad-hoc example and not a real test case.

[...]

Toggle quote (2 lines)
> I am not able to imagine an incomplete file worse than \\x00.

Just to be sure, I mean: an incomplete integer.

For sure, any incomplete (unbalanced) sexp is breaking 'read', as the
example "foo or (1 or whatever else; as you are correctly pointing.
But since the cache 'write' an integer, it means it would be an
incomplete integer.


Cheers,
simon
M
M
Maxime Devos wrote on 27 May 2022 16:02
(name . zimoun)(address . zimon.toutoune@gmail.com)
ab7445797a8f02495ed957861b22ca38459df45f.camel@telenet.be
Toggle quote (4 lines)
> > The difference is that ^R is interpreted as a symbol, whereas "foo
> > cannot be interpreted as anything at all by 'read'.
> I do not understand what you mean

^R is interpreted as a symbol:
(symbol? (call-with-input-string "\x12" read)).
"foo" is interpreted as a string:
(string? (call-with-input-string "\"foo\"" read))
"foo without a terminating string cannot be interpreted at all:
(call-with-input-string "\"foo" read)

Toggle quote (2 lines)
> and I think you are overengineering.

It's not any more overengineering than catching not-an-integer IMO.

AFAICT, this does not find the definition of overengineering I found
on Wikipedia.

Also, I do not understand the resistance -- I have a simple proposal
for generalising your patch to more failure modes, with a demonstration
and test case (see the file "a") on when it is necessary and a
proposed implementation.

zimoun schreef op vr 27-05-2022 om 15:23 [+0200]:
If you are able to produce a corrupted file which breaks "guix
time-machine", then we can examine.  Else let move on. :-)

I previously produced the corrupted file, see the file "a".

I am not willing to deliberately corrupt my file system for this,
especially when I can just give a synthetic example of corrupted
file (see the file "a") and especially since making a synthetic
example is much simpler and faster.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpDZ9RccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7icOAPwPHcd5Z6PsdU6lw8lk7HcX1iFG
kLtK37QV5tnJSMZYHwD+ObFnnfbUhNH1mxjxADTLaVFS/XZ6DoxyQHuB39vX6QI=
=RLZx
-----END PGP SIGNATURE-----


Z
Z
zimoun wrote on 27 May 2022 17:46
[PATCH v2] cache: Catch valid integer for 'last-expiry-cleanup'.
(address . 55673@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20220527154652.700549-1-zimon.toutoune@gmail.com

* guix/cache.scm (maybe-remove-expired-cache-entries)[last-expiry-date]: Check
if the date is a valid integer.
* tests/cache.scm: Test it.
---
guix/cache.scm | 16 ++++++++++++----
tests/cache.scm | 22 ++++++++++++++++++++++
2 files changed, 34 insertions(+), 4 deletions(-)

Toggle diff (77 lines)
diff --git a/guix/cache.scm b/guix/cache.scm
index 51009809bd..13f9e4a439 100644
--- a/guix/cache.scm
+++ b/guix/cache.scm
@@ -20,6 +20,7 @@ (define-module (guix cache)
#:use-module (srfi srfi-19)
#:use-module (srfi srfi-26)
#:use-module (ice-9 match)
+ #:use-module ((ice-9 textual-ports) #:select (get-string-all))
#:export (obsolete?
delete-file*
file-expiration-time
@@ -91,10 +92,17 @@ (define expiry-file
(string-append cache "/last-expiry-cleanup"))
(define last-expiry-date
- (catch 'system-error
- (lambda ()
- (call-with-input-file expiry-file read))
- (const 0)))
+ (let ((value (catch 'system-error
+ (lambda ()
+ (call-with-input-file expiry-file get-string-all))
+ (const "0"))))
+ (catch #t ; Handle value out of range (e.g., 1234567890 -> 12E4567890)
+ (lambda ()
+ ;; Handle empty or corrupted 'expiry-file' when 'write' below is
+ ;; interrupted before being complete (e.g., SIGINT with C-c) or when
+ ;; the filesystem crashes.
+ (or (string->number value) 0))
+ (const 0))))
(when (obsolete? last-expiry-date now cleanup-period)
(remove-expired-cache-entries (cache-entries cache)
diff --git a/tests/cache.scm b/tests/cache.scm
index 80b44d69aa..bd6fd64a22 100644
--- a/tests/cache.scm
+++ b/tests/cache.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2017, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2022 Simon Tournier <zimon.toutoune@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -74,6 +75,27 @@ (define-syntax-rule (test-cache-cleanup cache exp ...)
(lambda (port)
(display 0 port)))))
+(test-equal "maybe-remove-expired-cache-entries, empty cache"
+ '("a" "b" "c")
+ (test-cache-cleanup cache
+ (call-with-output-file (string-append cache "/last-expiry-cleanup")
+ (lambda (port)
+ (display "" port)))))
+
+(test-equal "maybe-remove-expired-cache-entries, corrupted cache"
+ '("a" "b" "c")
+ (test-cache-cleanup cache
+ (call-with-output-file (string-append cache "/last-expiry-cleanup")
+ (lambda (port)
+ (display "1\"34657890" port)))))
+
+(test-equal "maybe-remove-expired-cache-entries, corrupted cache of out range"
+ '("a" "b" "c")
+ (test-cache-cleanup cache
+ (call-with-output-file (string-append cache "/last-expiry-cleanup")
+ (lambda (port)
+ (display "12E4567890" port)))))
+
(test-end "cache")
;;; Local Variables:

base-commit: 38bf6c7d0cb588e8d4546db7d2e0bae2ec25183d
--
2.36.0
Z
Z
zimoun wrote on 27 May 2022 18:19
Re: [bug#55673] [PATCH] cache: Catch valid integer for 'last-expiry-cleanup'.
(name . Maxime Devos)(address . maximedevos@telenet.be)
CAJ3okZ2Z5P+xQHBzLSxASwWpfqEsxKmxe9S=cQP6JysvDWQc7g@mail.gmail.com
Hi,

On Fri, 27 May 2022 at 16:02, Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (5 lines)
> Also, I do not understand the resistance -- I have a simple proposal
> for generalising your patch to more failure modes, with a demonstration
> and test case (see the file "a") on when it is necessary and a
> proposed implementation.

I have sent a v2 using your proposal (which appears to me overcomplicated).

It is not resistance but pragmatic: the only case of interest is the
empty file, which happens -- all the others, I am still waiting at
least one bug report about them i.e., a user runs "guix time-machine"
and suddenly the file last-expiry-cleanup is corrupted and "guix
time-machine" unusable. Pragmatic because, for instance, from 2 to "
or from 8 to ( it is one bit-flip and thus 'read' would be easily
broken. I miss why such lengthy discussion about these theoretical
failures of last-expiry-cleanup when it is also true each time 'read'
is used, see least-authority or ui.scm etc. But I have never read a
word. Anyway.


Cheers,
simon
M
M
Maxime Devos wrote on 27 May 2022 19:23
(name . zimoun)(address . zimon.toutoune@gmail.com)
2ed60126c5ea7d7cffcad77082feba74ddab7517.camel@telenet.be
zimoun schreef op vr 27-05-2022 om 18:19 [+0200]:
Toggle quote (4 lines)
> I miss why such lengthy discussion about these theoretical
> failures of last-expiry-cleanup when it is also true each time 'read'
> is used, see least-authority or ui.scm etc.

(guix ui) cannot do anything about corruption except report the read
failure, whereas (guix cache) has a very strict file format so it is
feasible to detect whether it's corruption or just the user making a
typo (because those files aren't directly written by a user) and
additionally it can very easily handle the corruption.

For (guix authority), there is already a corruption detection mechanism
("guix gc --verify=contents") -- there even already is a repair
mechanism: "guix gc --verify=contents,repair".

Toggle quote (6 lines)
> It is not resistance but pragmatic: the only case of interest is
> the empty file, which happens -- all the others, I am still waiting
> at least one bug report about them i.e., a user runs "guix time-
> machine" and suddenly the file last-expiry-cleanup is corrupted and
> "guix time-machine" unusable.

* The general issue of file system corruption in Guix is already known
(the Guix daemon never calls fsync or sync except on the SQLite
database), though I don't know if a formal bug report exists about
that. There have been many bug reports on individual cases though.
* This bug report already exists: http://issues.guix.gnu.org/55638.
(You say the file system is not corrupted, but how would you know?
Even if not, the symptoms are almost identical.)
* I do not see the point of waiting for any known suffering users
reporting the bug before fixing the bug. Seems negligent to me
if the fix is easy and known, and not very pragmatic for those
future (or maybe current and shy) users. Also has a risk of rebase
conflicts, which does not seem pragmatic to me.

Greetings,
Maxime
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpEI+hccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7maLAP9BLkLrLMDf5+Uy5jugnE7WH/PX
8V9clB6T6gAePKwcjwEA0NUbqRToI/WCiqFiWs4AqzuIjUfZNUDwvOfp2tWZCgw=
=MEW/
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 27 May 2022 19:29
Re: [bug#55673] [PATCH v2] cache: Catch valid integer for 'last-expiry-cleanup'.
239d8888536d1573ecd466b96c8ff6170316daa2.camel@telenet.be
zimoun schreef op vr 27-05-2022 om 17:46 [+0200]:
Toggle quote (7 lines)
> +      (catch #t   ; Handle value out of range (e.g., 1234567890 -> 12E4567890)
> +        (lambda ()
> +          ;; Handle empty or corrupted 'expiry-file' when 'write' below is
> +          ;; interrupted before being complete (e.g., SIGINT with C-c) or when
> +          ;; the filesystem crashes.
> +          (or (string->number value) 0))

Why are 'stack-error' and 'out-of-memory' being caught?
I recommend using one of the regexes from string->generations
in (guix ui) instead to avoid catching too much.

Greetings,
Maxime.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYpEKixccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7j8zAQCzN+gPyiXMC1Hu9q8+zx2+RYnG
Cx/GfCkKQL0JR0PCdgD7BTxq9fa3ss+0U6893B5F7VGx3ZAAbh2zN/rw4kZk4wc=
=JaSK
-----END PGP SIGNATURE-----


Z
Z
zimoun wrote on 30 May 2022 15:07
[PATCH v3] cache: Catch invalid 'last-expiry-cleanup'.
(address . 55673@debbugs.gnu.org)(name . zimoun)(address . zimon.toutoune@gmail.com)
20220530130714.2142519-1-zimon.toutoune@gmail.com

* guix/cache.scm (maybe-remove-expired-cache-entries)[last-expiry-date]: Check
if the value is a valid integer.
* tests/cache.scm: Test it.
---
guix/cache.scm | 20 +++++++++++++++-----
tests/cache.scm | 22 ++++++++++++++++++++++
2 files changed, 37 insertions(+), 5 deletions(-)

Toggle diff (97 lines)
diff --git a/guix/cache.scm b/guix/cache.scm
index 51009809bd..9727a9eb58 100644
--- a/guix/cache.scm
+++ b/guix/cache.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2020, 2021 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2022 Simon Tournier <zimon.toutoune@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -17,9 +18,11 @@
;;; along with GNU Guix. If not, see <http://www.gnu.org/licenses/>.
(define-module (guix cache)
+ #:use-module ((guix utils) #:select (with-atomic-file-output))
#:use-module (srfi srfi-19)
#:use-module (srfi srfi-26)
#:use-module (ice-9 match)
+ #:use-module ((ice-9 textual-ports) #:select (get-string-all))
#:export (obsolete?
delete-file*
file-expiration-time
@@ -91,10 +94,17 @@ (define expiry-file
(string-append cache "/last-expiry-cleanup"))
(define last-expiry-date
- (catch 'system-error
- (lambda ()
- (call-with-input-file expiry-file read))
- (const 0)))
+ (let ((str (catch 'system-error
+ (lambda ()
+ (call-with-input-file expiry-file get-string-all))
+ (const "0"))))
+ ;; Handle empty or corrupted 'expiry-file' when 'write' below is
+ ;; interrupted before being complete (e.g., SIGINT with C-c) or when
+ ;; the filesystem crashes.
+ (if (or (string-index str #\e) ; Handle value out of range
+ (string-index str #\E)) ; (e.g., 1234567890 -> 12E4567890)
+ 0
+ (or (string->number str) 0))))
(when (obsolete? last-expiry-date now cleanup-period)
(remove-expired-cache-entries (cache-entries cache)
@@ -103,7 +113,7 @@ (define last-expiry-date
#:delete-entry delete-entry)
(catch 'system-error
(lambda ()
- (call-with-output-file expiry-file
+ (with-atomic-file-output expiry-file
(cute write (time-second now) <>)))
(lambda args
;; ENOENT means CACHE does not exist.
diff --git a/tests/cache.scm b/tests/cache.scm
index 80b44d69aa..bd6fd64a22 100644
--- a/tests/cache.scm
+++ b/tests/cache.scm
@@ -1,5 +1,6 @@
;;; GNU Guix --- Functional package management for GNU
;;; Copyright © 2017, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2022 Simon Tournier <zimon.toutoune@gmail.com>
;;;
;;; This file is part of GNU Guix.
;;;
@@ -74,6 +75,27 @@ (define-syntax-rule (test-cache-cleanup cache exp ...)
(lambda (port)
(display 0 port)))))
+(test-equal "maybe-remove-expired-cache-entries, empty cache"
+ '("a" "b" "c")
+ (test-cache-cleanup cache
+ (call-with-output-file (string-append cache "/last-expiry-cleanup")
+ (lambda (port)
+ (display "" port)))))
+
+(test-equal "maybe-remove-expired-cache-entries, corrupted cache"
+ '("a" "b" "c")
+ (test-cache-cleanup cache
+ (call-with-output-file (string-append cache "/last-expiry-cleanup")
+ (lambda (port)
+ (display "1\"34657890" port)))))
+
+(test-equal "maybe-remove-expired-cache-entries, corrupted cache of out range"
+ '("a" "b" "c")
+ (test-cache-cleanup cache
+ (call-with-output-file (string-append cache "/last-expiry-cleanup")
+ (lambda (port)
+ (display "12E4567890" port)))))
+
(test-end "cache")
;;; Local Variables:

base-commit: 38bf6c7d0cb588e8d4546db7d2e0bae2ec25183d
--
2.36.0
Z
Z
zimoun wrote on 30 May 2022 15:09
Re: [bug#55673] [PATCH v2] cache: Catch valid integer for 'last-expiry-cleanup'.
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 55673@debbugs.gnu.org)
CAJ3okZ1=H9M7H9mzPrrLc=8NW+cWr2hf=0-N9ambguS-RTpE3A@mail.gmail.com
On Fri, 27 May 2022 at 19:33, Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (4 lines)
> Why are 'stack-error' and 'out-of-memory' being caught?
> I recommend using one of the regexes from string->generations
> in (guix ui) instead to avoid catching too much.

Well, I have sent a v3. Note that that 'string->generations' does not
catch the value out of range we are talking.

Toggle snippet (27 lines)
$ guix package --list-generations=12E4567890
Backtrace:
9 (primitive-load "/home/simon/.config/guix/current/bin/guix")
In guix/ui.scm:
2230:7 8 (run-guix . _)
2193:10 7 (run-guix-command _ . _)
In ice-9/boot-9.scm:
1752:10 6 (with-exception-handler _ _ #:unwind? _ #:unwind-for-type _)
In guix/scripts/package.scm:
799:7 5 (_)
In ice-9/boot-9.scm:
1747:15 4 (with-exception-handler #<procedure 7f97f7728150 at
ice-9/boot-9.scm:1831:7 (exn)> _ …)
In guix/scripts/package.scm:
810:16 3 (_)
In guix/ui.scm:
1887:9 2 (matching-generations "12E4567890"
"/var/guix/profiles/per-user/simon/guix-profile" …)
1760:13 1 (string->generations _)
In ice-9/boot-9.scm:
1685:16 0 (raise-exception _ #:continuable? _)

ice-9/boot-9.scm:1685:16: In procedure raise-exception:
In procedure string->number: Value out of range: 4567890


Cheers,
simon
L
L
Ludovic Courtès wrote on 4 Jun 2022 12:11
Re: bug#55673: [PATCH] cache: Catch valid integer for 'last-expiry-cleanup'.
(name . zimoun)(address . zimon.toutoune@gmail.com)(address . 55673-done@debbugs.gnu.org)
87k09wg2ld.fsf_-_@gnu.org
Hi,

zimoun <zimon.toutoune@gmail.com> skribis:

Toggle quote (6 lines)
>
> * guix/cache.scm (maybe-remove-expired-cache-entries)[last-expiry-date]: Check
> if the value is a valid integer.
> * tests/cache.scm: Test it.

I simplified it a bit and pushed as
104b4e25ab7da5697f2f6f1ddfdd4955f05afece, thanks!

Ludo’.
Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 55673@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 55673
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch