Repository: webdesus/fs_extra (language: rust)

Issue:
`dir::get_size` returns wrong size for symlinks
## Summary

If the directory contains symlinks, then [`dir::get_size`](https://github.com/webdesus/fs_extra/blob/5fccd175d87e76fba06a8e6f3381d9dc18ef1ed7/src/dir.rs#L761-L801) follows the symlinks and counts the sizes of those files too. If the targets of those symlinks are inside that same directory structure, then it effectively means the file sizes of some files are counted twice. Or if the targets of those symlinks are not inside that directory structure, then it means the returned size isn't really that of the requested directory, but also includes that of arbitrary other locations on the filesystem, which doesn't seem desirable.

## Steps to reproduce

1. `mkdir test-dir`
2. `truncate -s 1M test-dir/large-file`
3. `ln -sr test-dir/large-file test-dir/symlink-to-large-file`
4. Run the following (using `fs_extra` `v1.2.0`):
   ```rust
   fn main() {
       let size = fs_extra::dir::get_size("test-dir").expect("IO error calculating size");
       println!("size = {size}");
   }
   ```

## Expected

`fs_extra::dir::get_size()` returns the same size as `ls` would, eg:

```
ls -al test-dir/
total 0
drwxr-xr-x 4 emorley staff     128 Jul  1 11:42 .
drwxr-xr-x 9 emorley staff     288 Jul  1 11:42 ..
-rw-r--r-- 1 emorley staff 1048576 Jul  1 11:41 large-file
lrwxr-xr-x 1 emorley staff      10 Jul  1 11:42 symlink-to-large-file -> large-file
```

...so 1048576 + 10 (for the symlink) + 128 (for the directory itself) = 1048714

ie something like:

```
$ cargo run -q
size = 1048714
```

## Actual

`fs_extra::dir::get_size()` follows the symlink, so counts the size of that file twice. It also doesn't seem to count the size of the directory entry itself.

```
$ cargo run -q
size = 2097152
```

## Notes

I believe the cause is that `fs_extra::dir::get_size()` uses [`std::fs::metadata`](https://doc.rust-lang.org/std/fs/fn.metadata.html) not [`std::fs::symlink_metadata`](https://doc.rust-lang.org/std/fs/fn.symlink_metadata.html), and that the former follows symlinks. Also, use of `std::fs::metadata` is actually unnecessary since the [`std::fs::DirEntry`](https://doc.rust-lang.org/std/fs/struct.DirEntry.html) returned by [`std::fs::read_dir`](https://doc.rust-lang.org/std/fs/fn.read_dir.html) already has the metadata we need.


Discussion / hints:

Fix patch:
diff --git a/src/dir.rs b/src/dir.rs
index 7915f79..a5d1112 100644
--- a/src/dir.rs
+++ b/src/dir.rs
@@ -269,6 +269,7 @@ where
     let metadata = path.metadata()?;
     get_details_entry_with_meta(path, config, metadata)
 }
+
 fn get_details_entry_with_meta<P>(
     path: P,
     config: &HashSet<DirEntryAttr>,
@@ -758,11 +759,13 @@ where
     })
 }
 
-/// Returns the size of the file or directory in bytes
+/// Returns the size of the file or directory in bytes.(!important: folders size not count)
 ///
 /// If used on a directory, this function will recursively iterate over every file and every
 /// directory inside the directory. This can be very time consuming if used on large directories.
 ///
+/// Does not follow symlinks.
+///
 /// # Errors
 ///
 /// This function will return an error in the following situations, but is not limited to just
@@ -784,20 +787,32 @@ pub fn get_size<P>(path: P) -> Result<u64>
 where
     P: AsRef<Path>,
 {
-    let mut result = 0;
+    // Using `fs::symlink_metadata` since we don't want to follow symlinks,
+    // as we're calculating the exact size of the requested path itself.
+    let path_metadata = path.as_ref().symlink_metadata()?;
 
-    if path.as_ref().is_dir() {
+    let mut size_in_bytes = 0;
+
+    if path_metadata.is_dir() {
         for entry in read_dir(&path)? {
-            let _path = entry?.path();
-            result += _path.metadata()?.len();
-            if _path.is_dir() {
-                result += get_size(_path)?;
+            let entry = entry?;
+            // `DirEntry::metadata` does not follow symlinks (unlike `fs::metadata`), so in the
+            // case of symlinks, this is the size of the symlink itself, not its target.
+            let entry_metadata = entry.metadata()?;
+
+            if entry_metadata.is_dir() {
+                // The size of the directory entry itself will be counted inside the `get_size()` call,
+                // so we intentionally don't also add `entry_metadata.len()` to the total here.
+                size_in_bytes += get_size(entry.path())?;
+            } else {
+                size_in_bytes += entry_metadata.len();
             }
         }
     } else {
-        result = path.as_ref().metadata()?.len();
+        size_in_bytes = path_metadata.len();
     }
-    Ok(result)
+
+    Ok(size_in_bytes)
 }
 
 /// Copies the directory contents from one place to another using recursive method,


Test patch:
diff --git a/tests/dir.rs b/tests/dir.rs
index 0ab7b33..2647066 100644
--- a/tests/dir.rs
+++ b/tests/dir.rs
@@ -1,5 +1,5 @@
 use std::collections::HashSet;
-use std::fs::read_dir;
+use std::fs::{self, read_dir};
 use std::path::{Path, PathBuf};
 use std::sync::mpsc::{self, TryRecvError};
 use std::thread;
@@ -70,6 +70,22 @@ fn get_dir_size() -> u64 {
         .len()
 }
 
+#[cfg(unix)]
+fn create_file_symlink<P: AsRef<Path>, Q: AsRef<Path>>(
+    original: P,
+    link: Q,
+) -> std::io::Result<()> {
+    std::os::unix::fs::symlink(original.as_ref(), link.as_ref())
+}
+
+#[cfg(windows)]
+fn create_file_symlink<P: AsRef<Path>, Q: AsRef<Path>>(
+    original: P,
+    link: Q,
+) -> std::io::Result<()> {
+    std::os::windows::fs::symlink_file(original.as_ref(), link.as_ref())
+}
+
 const TEST_FOLDER: &'static str = "./tests/temp/dir";
 
 #[test]
@@ -2821,22 +2837,42 @@ fn it_get_folder_size() {
 
     let mut file1 = path.clone();
     file1.push("test1.txt");
-    fs_extra::file::write_all(&file1, "content1").unwrap();
+    fs_extra::file::write_all(&file1, &"A".repeat(100)).unwrap();
     assert!(file1.exists());
 
     let mut sub_dir_path = path.clone();
     sub_dir_path.push("sub");
     create(&sub_dir_path, true).unwrap();
+
     let mut file2 = sub_dir_path.clone();
     file2.push("test2.txt");
-    fs_extra::file::write_all(&file2, "content2").unwrap();
+    fs_extra::file::write_all(&file2, &"B".repeat(300)).unwrap();
     assert!(file2.exists());
 
-    let result = get_size(&path).unwrap();
+    let symlink_file = sub_dir_path.join("symlink_file.txt");
+
+    // Rust stdlib APIs for creating a symlinked file only exist for Unix and Windows.
+    #[cfg(any(unix, windows))]
+    {
+        // Only passing the filename since we want this to be a relative symlink.
+        create_file_symlink("test2.txt", &symlink_file).unwrap();
+        assert!(symlink_file.exists());
+    }
+
+    // Total size comprises of:
+    // - 100 bytes for the standard file "test1.txt"
+    // - 300 bytes for the standard file "test2.txt"
+    // - (On supported platforms) 1 x symlink whose whose size varies by filesystem, so is dynamically calculated.
+    let mut expected_size = 100 + 300;
 
-    let test_size: u64 = 16 + get_dir_size();
+    if symlink_file.exists() {
+        // `fs::symlink_metadata` does not follow symlinks, so this is the size of the symlink itself, not its target.
+        expected_size += fs::symlink_metadata(&symlink_file).unwrap().len();
+    }
+
+    let result = get_size(&path).unwrap();
 
-    assert_eq!(test_size, result);
+    assert_eq!(expected_size, result);
 }
 
 #[test]
diff --git a/tests/lib.rs b/tests/lib.rs
index 4917f32..2d8bfd0 100644
--- a/tests/lib.rs
+++ b/tests/lib.rs
@@ -59,16 +59,6 @@ where
     true
 }
 
-// Returns the size of a directory. On Linux with ext4 this can be about 4kB.
-// Since the directory size can vary, we need to calculate is dynamically.
-fn get_dir_size() -> u64 {
-    std::fs::create_dir_all("./tests/temp").expect("Couldn't create test folder");
-
-    std::fs::metadata("./tests/temp")
-        .expect("Couldn't receive metadata of tests/temp folder")
-        .len()
-}
-
 const TEST_FOLDER: &'static str = "./tests/temp/lib";
 
 #[test]
@@ -922,10 +912,10 @@ fn it_copy_progress_work() {
             Ok(process_info) => {
                 if process_info.file_name == "file2.txt" {
                     assert_eq!(9, process_info.file_total_bytes);
-                    assert_eq!(get_dir_size() + 41, process_info.total_bytes);
+                    assert_eq!(41, process_info.total_bytes);
                 } else if process_info.file_name == "file1.txt" {
                     assert_eq!(8, process_info.file_total_bytes);
-                    assert_eq!(get_dir_size() + 41, process_info.total_bytes);
+                    assert_eq!(41, process_info.total_bytes);
                 }
             }
             Err(TryRecvError::Disconnected) => {
@@ -1033,14 +1023,14 @@ fn it_copy_with_progress_work_dif_buf_size() {
             assert_eq!(i * 2, process_info.file_bytes_copied);
             assert_eq!(i * 2, process_info.copied_bytes);
             assert_eq!(8, process_info.file_total_bytes);
-            assert_eq!(get_dir_size() + 40, process_info.total_bytes);
+            assert_eq!(40, process_info.total_bytes);
         }
         for i in 1..5 {
             let process_info: TransitProcess = rx.recv().unwrap();
             assert_eq!(i * 2 + 8, process_info.copied_bytes);
             assert_eq!(i * 2, process_info.file_bytes_copied);
             assert_eq!(8, process_info.file_total_bytes);
-            assert_eq!(get_dir_size() + 40, process_info.total_bytes);
+            assert_eq!(40, process_info.total_bytes);
         }
 
         match result {
@@ -1055,14 +1045,14 @@ fn it_copy_with_progress_work_dif_buf_size() {
         assert_eq!(i, process_info.file_bytes_copied);
         assert_eq!(i, process_info.copied_bytes);
         assert_eq!(8, process_info.file_total_bytes);
-        assert_eq!(get_dir_size() + 40, process_info.total_bytes);
+        assert_eq!(40, process_info.total_bytes);
     }
     for i in 1..9 {
         let process_info: TransitProcess = rx.recv().unwrap();
         assert_eq!(i + 8, process_info.copied_bytes);
         assert_eq!(i, process_info.file_bytes_copied);
         assert_eq!(8, process_info.file_total_bytes);
-        assert_eq!(get_dir_size() + 40, process_info.total_bytes);
+        assert_eq!(40, process_info.total_bytes);
     }
 
     match result {
@@ -2385,10 +2375,10 @@ fn it_move_progress_work() {
             Ok(process_info) => {
                 if process_info.file_name == "file2.txt" {
                     assert_eq!(9, process_info.file_total_bytes);
-                    assert_eq!(get_dir_size() + 41, process_info.total_bytes);
+                    assert_eq!(41, process_info.total_bytes);
                 } else if process_info.file_name == "file1.txt" {
                     assert_eq!(8, process_info.file_total_bytes);
-                    assert_eq!(get_dir_size() + 41, process_info.total_bytes);
+                    assert_eq!(41, process_info.total_bytes);
                 }
             }
             Err(TryRecvError::Disconnected) => {