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

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

[zimoun]

Fixes <http://issues.guix.gnu.org/55638>.

* 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(-)

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

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

[Maxime Devos]

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

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'.

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

[zimoun]

Hi Maxime,

On Fri, 27 May 2022 at 11:54, Maxime Devos <maximedevos@telenet.be> wrote:
> 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.


> 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

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

[Maxime Devos]

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

zimoun schreef op vr 27-05-2022 om 12:28 [+0200]:
> > 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

[zimoun]

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

> 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'.

--8<---------------cut here---------------start------------->8---
$ 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]
--8<---------------cut here---------------end--------------->8---

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

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

[Maxime Devos]

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

zimoun schreef op vr 27-05-2022 om 13:39 [+0200]:
> 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

[zimoun]

Hi Maxime,

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

> 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

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

[Maxime Devos]

[-- Attachment #1.1: Type: text/plain, Size: 1181 bytes --]

zimoun schreef op vr 27-05-2022 om 14:40 [+0200]:
> > 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.

[-- Attachment #1.2: a --]
[-- Type: text/plain, Size: 4 bytes --]

"foo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

[zimoun]

Hi Maxime,

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.

--8<---------------cut here---------------start------------->8---
$ 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.
--8<---------------cut here---------------end--------------->8---

As previously mentioned, the patch fixes:

 - empty file
 - non-integer

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


> 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

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

[zimoun]

On Fri, 27 May 2022 at 15:23, zimoun <zimon.toutoune@gmail.com> wrote:
> 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.

[...]

> 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

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

[Maxime Devos]

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

> > 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)

> 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

[zimoun]

Hi,

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

> 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

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

[Maxime Devos]

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

zimoun schreef op vr 27-05-2022 om 18:19 [+0200]:
> 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".

> 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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

[Maxime Devos]

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

zimoun schreef op vr 27-05-2022 om 12:28 [+0200]:
> > 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

[zimoun]

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.


Cheers,
simon

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

[Maxime Devos]

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

zimoun schreef op vr 27-05-2022 om 13:24 [+0200]:
> 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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55673] [PATCH v2] cache: Catch valid integer for 'last-expiry-cleanup'.

[zimoun]

Fixes <http://issues.guix.gnu.org/55638>.

* 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(-)

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

[bug#55673] [PATCH v2] cache: Catch valid integer for 'last-expiry-cleanup'.

[Maxime Devos]

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

zimoun schreef op vr 27-05-2022 om 17:46 [+0200]:
> +      (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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55673] [PATCH v2] cache: Catch valid integer for 'last-expiry-cleanup'.

[zimoun]

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

> 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.

--8<---------------cut here---------------start------------->8---
$ 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
--8<---------------cut here---------------end--------------->8---


Cheers,
simon

[bug#55673] [PATCH v3] cache: Catch invalid 'last-expiry-cleanup'.

[zimoun]

Fixes <http://issues.guix.gnu.org/55638>.

* 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(-)

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

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

[Ludovic Courtès]

Hi,

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

> Fixes <http://issues.guix.gnu.org/55638>.
>
> * 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’.