sifive / wake

The SiFive wake build tool
Other
86 stars 28 forks source link

File-edited check doesn't check permissions on subsequent invocations #1595

Open ag-eitilt opened 2 months ago

ag-eitilt commented 2 months ago

Split from #1589 but not covered in the meeting: If the user changes the permissions of a file previously output by Wake but doesn't edit its contents, Wake doesn't detect that difference and assumes that the cached Job may still be used as-is:

$ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob'
Job 617
$ ls -l test.txt
-rw-r--r-- 1 sammay sammay 4 Jul 10 14:56 test.txt
$ chmod -w test.txt
$ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob'
Job 617
$ ls -l test.txt
-r--r--r-- 1 sammay sammay 4 Jul 10 14:56 test.txt

(Note the same Job ID but different permissions.)

This might not actually be much of a problem in practice, as if a later Job tries to write to that same file Wake itself is supposed to complain, permissions or not (however, note that for some reason in my first invocation below it's the Job which is failing rather than a File output by multiple jobs panic being thrown), but it is easily possible to create a Job which fails if it doesn't have write permissions in such a way it wouldn't produce any output file, and therefore wouldn't trigger the panic. Theoretically, this is no different from what's possible with the stat prim/function, but that at least is known to be if not unsafe, at least advanced-audience.

$ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob | getJobOutputs |> (makePlan "edit" _ "echo edit >> test.txt" | runJob | getJobOutputs)'
Fail (Error "Non-zero exit status (Exited 1) for job 748: 'edit'" Nil)
$ chmod -w test.txt 
$ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob | getJobOutputs |> (makePlan "edit" _ "echo edit >> test.txt" | runJob | getJobOutputs)'
/usr/bin/dash: 1: cannot create test.txt: Permission denied
Fail (Error "Non-zero exit status (Exited 2) for job 755: 'edit'" Nil)

(I'm not actually sure what's going on there; given the current limitations of the checker, I would have expected the first edit to work within the sandbox but to panic immediately in the process of exiting that sandbox, and only the second to have one of the "successfully completed with a failure" cases we laugh about.)

Note that this isn't a problem with chmod -r as (presumably) the lack of a readable hash triggers the Job to be rerun, and similar to #1594, the file to be recreated; it might be a problem if the executable bit is the one being toggled.

$ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob'
Job 767
$ chmod -r test.txt 
$ ls -l test.txt 
--w------- 1 sammay sammay 4 Jul 10 15:18 test.txt
$ wake -x 'makePlan "test" Nil "echo -n test > test.txt" | runJob'
Job 780
$ ls -l test.txt 
-rw-r--r-- 1 sammay sammay 4 Jul 10 15:19 test.txt

All confusion aside, the third example is what I'd actually expect: the file after the second Job is exactly as it was (modtime aside) immediately after the first Job. The first example is illustrating a bug where Wake's reproducibility checks aren't actually tight enough to ensure that assumption, and the second was meant to illustrate a potential panic/ignorable-Fail break in reproducibility leading from that but is actually accidentally illustrating some bug I'm not even sure how to categorize.