sending an empty directory produces a BLAKE3 `hazmat` assertion failure

[oconnor663]

This is with sendme--version 0.25.0. In one terminal:

$ cd `mktemp -d`
$ sendme send .
...

In another:

$ sendme receive ...
getting collection 321465e756fab26fd96d9a252ca91111d9674e5cbb8756c79173c5cb3e5c9a0e 1 files, 1.52 MiB
[3/3] Downloading 3 blob(s)
⠁ [00:00:00] [-------------------------------------------------------------------------------------] 0 B/1.52 MiB 0 B/s                            
thread 'main' panicked at /home/jacko/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/blake3-1.8.1/src/hazmat.rs:237:9:
assertion `left != right` failed: empty subtrees are never valid
  left: 0
 right: 0

[oconnor663]

Trying the same thing on main I see Remote behaved in a non-compliant way instead of the panic. I'm not sure whether that's a different error, or just different output for the same error.

[rklaehn]

This must be related to the fact that you are creating the temp files for sendme in the same dir as where you are.

mkdir tmp
sendme send tmp
...

does not have the issue.

But it should not happen. I will take a closer look tomorrow.

[rklaehn]

pretty sure it is unrelated to the hazmat changes. very likely a bug on our side.

[rklaehn]

Interesting bug. So things get screwed up because we create a database in the send dir, and if this is the current dir, we also try to scan the database itself. So basically we are sending data that is mutated. This can not work. The sender detects that the data is corrupt and stops sending.

On the recv side this caused a short read (something that should have a size of x != 0 was read with a size 0. And then we feed this into the hasher, and it panics because hashing a blob of size 0 as a non root hash must be wrong.

Before it would just produce a wrong value, leading to validation failing.

[rklaehn]

I don't mind the panic. This is obvious misuse of the hasher. We have to fix this on our side!

[oconnor663]

Yeah my guess is that this was a pre-existing bug, and the aggressive asserts in the new hazmat API just uncovered it? I went back and forth about whether they should be debug_assert! or assert!, and I ended up going with assert!. Does that seem like the right call?

[rklaehn]

Yeah my guess is that this was a pre-existing bug, and the aggressive asserts in the new hazmat API just uncovered it? I went back and forth about whether they should be debug_assert! or assert!, and I ended up going with assert!. Does that seem like the right call?

This is exactly it. There was a preexisting bug that led to the exact wanted behaviour - hash did not match, content was rejected at exactly the right offset. So I did not notice it - basically the thing worked as designed, but for the wrong reason.

I will fix this today (really needed some downtime over the weekend). I am myself a bit unsure about whether this should be an assert! or debug_assert!. In general I like functions to either return an error or be total, but this is a special case.

I would say given the general philosophy of BLAKE3, if doing this wrong has security implications, or if there is even the slightest doubt whether it could have security implications, it should be an assert!. If not, it should be a debug_assert!.