Fix: Recursively calculate directory size in size_of_dir

[maradini77]

Fixed size_of_dir_entry to recursively calculate the size of directories. Previously, the function only returned the metadata size of directory entries (typically a few KB), completely ignoring files within subdirectories. This caused size_of_dir to return incorrect sizes for any directory tree with nested folders.

The fix checks if an entry is a directory and recursively calls size_of_dir to sum up all file sizes within the directory tree.

[cla-assistant]

All committers have signed the CLA.

[michaelsproul]

Please rebase your change on unstable.

[michaelsproul]

I was a little hesitant to merge this change, because:

1. We don't need it. The structure of the beacon chain database directory is completely flat (files inside a single directory).
2. I was concerned that this change could open the door to stack overflows or infinite loops.

Concern (2) is more important. Looking at the docs for DirEntry::metadata and Metadata::is_dir we see that it would not return true for a symlinked directory. This prevents the infinite loop attack.

The risk of a deeply nested directory creating a DoS via stack overflow (or very long run time) still exists, although it would require the datadir to be writable by an attacker, which is likely only possible if permissions are incorrectly set, or there is a remote code execution vulnerability (which creates more serious problems beyond a simple DoS).

I'm leaning towards saying that this slightly increased risk is acceptable, given that it can be mitigated by setting file permissions correctly. This is despite the positive impact being low (no bug in LH's current behaviour). I think fixing the behaviour for the general case is still valuable, as it removes a footgun and saves us from potentially introducing a bug in the case where the assumption about a flat data directory is no longer true.

[michaelsproul]

@maradini77 Do you have any thoughts on the above?

[maradini77]

@michaelsproul Thanks for the careful review. Agreed: symlinks won’t cause an infinite loop because is_dir is false on them. I added recursion to avoid hidden bugs if the data directory ever stops being flat (e.g., future tools/plugins). To reduce DoS risk from deep nesting, I can switch to an iterative walk with a depth/entry cap and skip symlinks. Would you be comfortable if I add those protections?

[michaelsproul]

@maradini77 Yeah let's make it iterative and capped, sounds good!

[maradini77]

@maradini77 Yeah let's make it iterative and capped, sounds good!

@michaelsproul updated

[mergify]

Some required checks have failed. Could you please take a look @maradini77? 🙏

[maradini77]

@michaelsproul fmt fuxed

[mergify]

Some required checks have failed. Could you please take a look @maradini77? 🙏

[macladson]

Just noticed an inconsistency with the comments. Also, I think some tests would be useful to here to make sure that the behaviour we want is being enforced (particularly with regards to symlinks).

tempfile is a good crate to use for testing this and is already used elsewhere so it's already in our workspace dependencies. You can also create symlinks with the symlink function: https://doc.rust-lang.org/std/os/unix/fs/fn.symlink.html

[macladson]

@maradini77 are you able to write some tests for this function?

Something like this:

#[cfg(test)]
mod tests {
    use super::*;
    use tempfile::TempDir;

    #[test]
    fn size_of_dir_empty() {
        let temp_dir = TempDir::new().unwrap();
        let size = size_of_dir(temp_dir.path());
        assert_eq!(size, 0);
    }

    // ... etc
}