Skip to content

Fix an infinite loop if the input stream and output stream are the same#11384

Merged
WindSoilder merged 4 commits intonushell:mainfrom
nibon7:fix_save
Dec 24, 2023
Merged

Fix an infinite loop if the input stream and output stream are the same#11384
WindSoilder merged 4 commits intonushell:mainfrom
nibon7:fix_save

Conversation

@nibon7
Copy link
Copy Markdown
Contributor

@nibon7 nibon7 commented Dec 21, 2023

Description

Fixes #11382

User-Facing Changes

  • before
nushell/test (109f629) [✘?]open hello.md
hello
nushell/test (109f629) [✘?]ls hello.md | get size
╭───┬─────╮
│ 0 │ 6 B │
╰───┴─────╯
nushell/test (109f629) [✘?]open --raw hello.md | prepend "world" | save --raw --force hello.md
^C
nushell/test (109f629) [✘?]ls hello.md | get size
╭───┬─────────╮
│ 0 │ 2.8 GiB │
╰───┴─────────╯
  • after
nushell/test on  fix_save [✘!?⇡]open hello.md | prepend "hello" | save --force hello.md
nushell/test on  fix_save [✘!?⇡]open --raw hello.md | prepend "hello" | save --raw --force ../test/hello.md
Error:   × pipeline input and output are same file
   ╭─[entry #4:1:1]
 1 │ open --raw hello.md | prepend "hello" | save --raw --force ../test/hello.md
   ·                                                           ────────┬───────
   ·                                                                   ╰── can't save output to '/data/source/nushell/test/hello.md' while it's being reading
   ╰────
  help: you should change output path


nushell/test on  fix_save [✘!?⇡]open hello | prepend "hello" | save --force hello
Error:   × pipeline input and output are same file
   ╭─[entry #5:1:1]
 1 │ open hello | prepend "hello" | save --force hello
   ·                                            ──┬──
   ·                                              ╰── can't save output to '/data/source/nushell/test/hello' while it's being reading
   ╰────
  help: you should change output path

Tests + Formatting

Make sure you've run and fixed any issues with these commands:

  • add commands::save::save_same_file_with_extension
  • add commands::save::save_same_file_without_extension
  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass (on Windows make sure to enable developer mode)
  • cargo run -- -c "use std testing; testing run-tests --path crates/nu-std" to run the tests for the standard library

After Submitting

@WindSoilder
Copy link
Copy Markdown
Contributor

WindSoilder commented Dec 21, 2023

Hi @nibon7 , Thanks for your pr!
Actually I'm hesitate about this change, it violates nushell's design that we should make use of stream feature as much as possible: #7590

Let's consider the following command:

seq 1 10000000 | save -f --raw a.txt

It will take too much memory to do this.

@nibon7
Copy link
Copy Markdown
Contributor Author

nibon7 commented Dec 21, 2023

It is a problem. Maybe we could detect whether input and output refer to the same file and return an error.

diff --git a/crates/nu-command/src/debug/metadata.rs b/crates/nu-command/src/debug/metadata.rs
index 256b5d80a..61991cb1d 100644
--- a/crates/nu-command/src/debug/metadata.rs
+++ b/crates/nu-command/src/debug/metadata.rs
@@ -86,6 +86,12 @@ impl Command for Metadata {
                         PipelineMetadata {
                             data_source: DataSource::HtmlThemes,
                         } => record.push("source", Value::string("into html --list", head)),
+                        PipelineMetadata {
+                            data_source: DataSource::FilePath(path),
+                        } => record.push(
+                            "source",
+                            Value::string(path.to_string_lossy().to_string(), head),
+                        ),
                     }
                 }
 
@@ -133,6 +139,12 @@ fn build_metadata_record(arg: &Value, metadata: Option<&PipelineMetadata>, head:
             PipelineMetadata {
                 data_source: DataSource::HtmlThemes,
             } => record.push("source", Value::string("into html --list", head)),
+            PipelineMetadata {
+                data_source: DataSource::FilePath(path),
+            } => record.push(
+                "source",
+                Value::string(path.to_string_lossy().to_string(), head),
+            ),
         }
     }
 
diff --git a/crates/nu-command/src/filesystem/open.rs b/crates/nu-command/src/filesystem/open.rs
index 3597ae2a2..114bd3623 100644
--- a/crates/nu-command/src/filesystem/open.rs
+++ b/crates/nu-command/src/filesystem/open.rs
@@ -1,10 +1,11 @@
 use nu_engine::{current_dir, eval_block, CallExt};
+use nu_path::expand_to_real_path;
 use nu_protocol::ast::Call;
 use nu_protocol::engine::{Command, EngineState, Stack};
 use nu_protocol::util::BufferedReader;
 use nu_protocol::{
-    Category, Example, IntoInterruptiblePipelineData, PipelineData, RawStream, ShellError,
-    Signature, Spanned, SyntaxShape, Type, Value,
+    Category, DataSource, Example, IntoInterruptiblePipelineData, PipelineData, PipelineMetadata,
+    RawStream, ShellError, Signature, Spanned, SyntaxShape, Type, Value,
 };
 use std::io::BufReader;
 
@@ -157,6 +158,7 @@ impl Command for Open {
                     };
 
                     let buf_reader = BufReader::new(file);
+                    let real_path = expand_to_real_path(path);
 
                     let file_contents = PipelineData::ExternalStream {
                         stdout: Some(RawStream::new(
@@ -168,7 +170,9 @@ impl Command for Open {
                         stderr: None,
                         exit_code: None,
                         span: call_span,
-                        metadata: None,
+                        metadata: Some(PipelineMetadata {
+                            data_source: DataSource::FilePath(real_path),
+                        }),
                         trim_end_newline: false,
                     };
                     let exts_opt: Option<Vec<String>> = if raw {
diff --git a/crates/nu-command/src/filesystem/save.rs b/crates/nu-command/src/filesystem/save.rs
index f61d0f286..11b2be074 100644
--- a/crates/nu-command/src/filesystem/save.rs
+++ b/crates/nu-command/src/filesystem/save.rs
@@ -1,11 +1,11 @@
 use nu_engine::current_dir;
 use nu_engine::CallExt;
-use nu_path::expand_path_with;
+use nu_path::{expand_path_with, expand_to_real_path};
 use nu_protocol::ast::{Call, Expr, Expression};
 use nu_protocol::engine::{Command, EngineState, Stack};
 use nu_protocol::{
-    Category, Example, PipelineData, RawStream, ShellError, Signature, Span, Spanned, SyntaxShape,
-    Type, Value,
+    Category, DataSource, Example, PipelineData, PipelineMetadata, RawStream, ShellError,
+    Signature, Span, Spanned, SyntaxShape, Type, Value,
 };
 use std::fs::File;
 use std::io::Write;
@@ -149,12 +149,35 @@ impl Command for Save {
                     res
                 }
             }
-            PipelineData::ListStream(ls, _)
+            PipelineData::ListStream(ls, pipeline_metadata)
                 if raw || prepare_path(&path, append, force)?.0.extension().is_none() =>
             {
-                // We need to consume the input stream here to prevent an infinite
-                // loop if the input stream and output stream are the same.
-                let values: Vec<Value> = ls.collect();
+                if let Some(PipelineMetadata {
+                    data_source: DataSource::FilePath(input_path),
+                }) = pipeline_metadata
+                {
+                    if expand_to_real_path(&path.item) == input_path {
+                        return Err(ShellError::GenericError {
+                            error: "pipeline input and output are same file".into(),
+                            msg: "can't save output".into(),
+                            span: Some(path.span),
+                            help: Some("you should change output path".into()),
+                            inner: vec![],
+                        });
+                    }
+
+                    if let Some(ref err_path) = stderr_path {
+                        if expand_to_real_path(&err_path.item) == input_path {
+                            return Err(ShellError::GenericError {
+                                error: "pipeline input and stderr are same file".into(),
+                                msg: "can't save stderr".into(),
+                                span: Some(err_path.span),
+                                help: Some("you should change stderr path".into()),
+                                inner: vec![],
+                            });
+                        }
+                    }
+                }
 
                 let (mut file, _) = get_files(
                     &path,
@@ -164,7 +187,7 @@ impl Command for Save {
                     err_append,
                     force,
                 )?;
-                for val in values.into_iter() {
+                for val in ls {
                     file.write_all(&value_to_bytes(val)?)
                         .map_err(|err| ShellError::IOError {
                             msg: err.to_string(),
diff --git a/crates/nu-protocol/src/pipeline_data.rs b/crates/nu-protocol/src/pipeline_data.rs
index 1b58057e7..8dda06dd7 100644
--- a/crates/nu-protocol/src/pipeline_data.rs
+++ b/crates/nu-protocol/src/pipeline_data.rs
@@ -4,6 +4,7 @@ use crate::{
     format_error, Config, ListStream, RawStream, ShellError, Span, Value,
 };
 use nu_utils::{stderr_write_all_and_flush, stdout_write_all_and_flush};
+use std::path::PathBuf;
 use std::sync::{atomic::AtomicBool, Arc};
 use std::thread;
 
@@ -62,6 +63,7 @@ pub struct PipelineMetadata {
 pub enum DataSource {
     Ls,
     HtmlThemes,
+    FilePath(PathBuf),
 }
 
 impl PipelineData {
open --raw a.md | prepend "hello" | save --force --raw ../test/a.md
Error:   × pipeline input and output are same file
   ╭─[entry #2:1:1]
 1 │ open --raw a.md | prepend "hello" | save --force --raw ../test/a.md
   ·                                                        ──────┬─────
   ·                                                              ╰── can't save output
   ╰────
  help: you should change output path


nushell/test on  fix_save [✘?⇡]

@fdncred fdncred marked this pull request as draft December 21, 2023 13:28
@fdncred
Copy link
Copy Markdown
Contributor

fdncred commented Dec 21, 2023

I think DataSource::FilePath is interesting. Making this draft for now.

@nibon7
Copy link
Copy Markdown
Contributor Author

nibon7 commented Dec 21, 2023

@WindSoilder any thoughts?

@WindSoilder
Copy link
Copy Markdown
Contributor

Thanks! I think it's a good idea too :-)

@nibon7 nibon7 marked this pull request as ready for review December 22, 2023 04:27
Copy link
Copy Markdown
Contributor

@WindSoilder WindSoilder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really thanks for a great improvement!

It gives me one more reason to use open rather than cat :-D

@WindSoilder WindSoilder merged commit aeffa18 into nushell:main Dec 24, 2023
@nibon7 nibon7 deleted the fix_save branch December 24, 2023 17:46
dmatos2012 pushed a commit to dmatos2012/nushell that referenced this pull request Feb 20, 2024
…me (nushell#11384)

# Description

Fixes nushell#11382 

# User-Facing Changes
* before

```console
nushell/test (109f629) [✘?]
❯ open hello.md
hello
nushell/test (109f629) [✘?]
❯ ls hello.md | get size
╭───┬─────╮
│ 0 │ 6 B │
╰───┴─────╯
nushell/test (109f629) [✘?]
❯ open --raw hello.md | prepend "world" | save --raw --force hello.md
^C
nushell/test (109f629) [✘?]
❯ ls hello.md | get size
╭───┬─────────╮
│ 0 │ 2.8 GiB │
╰───┴─────────╯
```

* after

```console
nushell/test on  fix_save [✘!?⇡]
❯ open hello.md | prepend "hello" | save --force hello.md
nushell/test on  fix_save [✘!?⇡]
❯ open --raw hello.md | prepend "hello" | save --raw --force ../test/hello.md
Error:   × pipeline input and output are same file
   ╭─[entry nushell#4:1:1]
 1 │ open --raw hello.md | prepend "hello" | save --raw --force ../test/hello.md
   ·                                                           ────────┬───────
   ·                                                                   ╰── can't save output to '/data/source/nushell/test/hello.md' while it's being reading
   ╰────
  help: you should change output path


nushell/test on  fix_save [✘!?⇡]
❯ open hello | prepend "hello" | save --force hello
Error:   × pipeline input and output are same file
   ╭─[entry nushell#5:1:1]
 1 │ open hello | prepend "hello" | save --force hello
   ·                                            ──┬──
   ·                                              ╰── can't save output to '/data/source/nushell/test/hello' while it's being reading
   ╰────
  help: you should change output path
```

# Tests + Formatting
Make sure you've run and fixed any issues with these commands:
- [x] add `commands::save::save_same_file_with_extension`
- [x] add `commands::save::save_same_file_without_extension`
- [x] `cargo fmt --all -- --check` to check standard code formatting
(`cargo fmt --all` applies these changes)
- [x] `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used`
to check that you're using the standard code style
- [x] `cargo test --workspace` to check that all tests pass (on Windows
make sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- [x] `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

# After Submitting
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Infinite save --raw

3 participants