Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent improper files (like /dev/random) from being used as file arguments #10733

Merged
merged 12 commits into from
Jun 18, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 24 additions & 13 deletions helix-term/src/application.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use helix_lsp::{
use helix_stdx::path::get_relative_path;
use helix_view::{
align_view,
document::DocumentSavedEventResult,
document::{DocumentOpenError, DocumentSavedEventResult},
editor::{ConfigEvent, EditorEvent},
graphics::Rect,
theme,
Expand Down Expand Up @@ -186,9 +186,14 @@ impl Application {
Some(Layout::Horizontal) => Action::HorizontalSplit,
None => Action::Load,
};
let doc_id = editor
.open(&file, action)
.context(format!("open '{}'", file.to_string_lossy()))?;
let doc_id = match editor.open(&file, action) {
// Ignore irregular files during application init.
Err(DocumentOpenError::IrregularFile) => {
nr_of_files -= 1;
continue;
}
a => a.context(format!("open '{}'", file.to_string_lossy())),
}?;
TiredTumblrina marked this conversation as resolved.
Show resolved Hide resolved
// with Action::Load all documents have the same view
// NOTE: this isn't necessarily true anymore. If
// `--vsplit` or `--hsplit` are used, the file which is
Expand All @@ -199,15 +204,21 @@ impl Application {
doc.set_selection(view_id, pos);
}
}
editor.set_status(format!(
"Loaded {} file{}.",
nr_of_files,
if nr_of_files == 1 { "" } else { "s" } // avoid "Loaded 1 files." grammo
));
// align the view to center after all files are loaded,
// does not affect views without pos since it is at the top
let (view, doc) = current!(editor);
align_view(doc, view, Align::Center);

// if all files were invalid, replace with empty buffer
if nr_of_files == 0 {
editor.new_file(Action::VerticalSplit);
} else {
editor.set_status(format!(
"Loaded {} file{}.",
nr_of_files,
if nr_of_files == 1 { "" } else { "s" } // avoid "Loaded 1 files." grammo
));
// align the view to center after all files are loaded,
// does not affect views without pos since it is at the top
let (view, doc) = current!(editor);
align_view(doc, view, Align::Center);
}
} else {
editor.new_file(Action::VerticalSplit);
}
Expand Down
1 change: 1 addition & 0 deletions helix-term/src/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ use crate::job::{self, Jobs};
use std::{
cmp::Ordering,
collections::{HashMap, HashSet},
error::Error,
fmt,
future::Future,
io::Read,
Expand Down
2 changes: 1 addition & 1 deletion helix-term/src/ui/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub use text::Text;

use helix_view::Editor;

use std::path::PathBuf;
use std::{error::Error, path::PathBuf};

pub fn prompt(
cx: &mut crate::commands::Context,
Expand Down
1 change: 1 addition & 0 deletions helix-view/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ toml = "0.8"
log = "~0.4"

parking_lot = "0.12.2"
thiserror = "1.0"
TiredTumblrina marked this conversation as resolved.
Show resolved Hide resolved


[target.'cfg(windows)'.dependencies]
Expand Down
29 changes: 23 additions & 6 deletions helix-view/src/document.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use anyhow::{anyhow, bail, Context, Error};
use anyhow::{anyhow, bail, Error};
use arc_swap::access::DynAccess;
use arc_swap::ArcSwap;
use futures_util::future::BoxFuture;
Expand All @@ -12,6 +12,7 @@ use helix_core::text_annotations::{InlineAnnotation, Overlay};
use helix_lsp::util::lsp_pos_to_pos;
use helix_stdx::faccess::{copy_metadata, readonly};
use helix_vcs::{DiffHandle, DiffProviderRegistry};
use thiserror;

use ::parking_lot::Mutex;
use serde::de::{self, Deserialize, Deserializer};
Expand All @@ -21,6 +22,7 @@ use std::cell::Cell;
use std::collections::HashMap;
use std::fmt::Display;
use std::future::Future;
use std::io;
use std::path::{Path, PathBuf};
use std::str::FromStr;
use std::sync::{Arc, Weak};
Expand Down Expand Up @@ -117,6 +119,14 @@ pub struct SavePoint {
revert: Mutex<Transaction>,
}

#[derive(Debug, thiserror::Error)]
pub enum DocumentOpenError {
#[error("path must be a regular file, simlink, or directory")]
IrregularFile,
#[error(transparent)]
IoError(#[from] io::Error),
}

pub struct Document {
pub(crate) id: DocumentId,
text: Rope,
Expand Down Expand Up @@ -380,7 +390,7 @@ fn apply_bom(encoding: &'static encoding::Encoding, buf: &mut [u8; BUF_SIZE]) ->
pub fn from_reader<R: std::io::Read + ?Sized>(
reader: &mut R,
encoding: Option<&'static Encoding>,
) -> Result<(Rope, &'static Encoding, bool), Error> {
) -> Result<(Rope, &'static Encoding, bool), io::Error> {
// These two buffers are 8192 bytes in size each and are used as
// intermediaries during the decoding process. Text read into `buf`
// from `reader` is decoded into `buf_out` as UTF-8. Once either
Expand Down Expand Up @@ -523,7 +533,7 @@ fn read_and_detect_encoding<R: std::io::Read + ?Sized>(
reader: &mut R,
encoding: Option<&'static Encoding>,
buf: &mut [u8],
) -> Result<(&'static Encoding, bool, encoding::Decoder, usize), Error> {
) -> Result<(&'static Encoding, bool, encoding::Decoder, usize), io::Error> {
let read = reader.read(buf)?;
let is_empty = read == 0;
let (encoding, has_bom) = encoding
Expand Down Expand Up @@ -685,11 +695,18 @@ impl Document {
encoding: Option<&'static Encoding>,
config_loader: Option<Arc<ArcSwap<syntax::Loader>>>,
config: Arc<dyn DynAccess<Config>>,
) -> Result<Self, Error> {
) -> Result<Self, DocumentOpenError> {
// If the path is not a regular file (e.g.: /dev/random) it should not be opened.
if path
.metadata()
.map_or(false, |metadata| !metadata.is_file())
{
return Err(DocumentOpenError::IrregularFile);
}

// Open the file if it exists, otherwise assume it is a new file (and thus empty).
let (rope, encoding, has_bom) = if path.exists() {
let mut file =
std::fs::File::open(path).context(format!("unable to open {:?}", path))?;
let mut file = std::fs::File::open(path)?;
from_reader(&mut file, encoding)?
} else {
let line_ending: LineEnding = config.load().default_line_ending.into();
Expand Down
6 changes: 4 additions & 2 deletions helix-view/src/editor.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use crate::{
align_view,
document::{DocumentSavedEventFuture, DocumentSavedEventResult, Mode, SavePoint},
document::{
DocumentOpenError, DocumentSavedEventFuture, DocumentSavedEventResult, Mode, SavePoint,
},
graphics::{CursorKind, Rect},
handlers::Handlers,
info::Info,
Expand Down Expand Up @@ -1616,7 +1618,7 @@ impl Editor {
}

// ??? possible use for integration tests
pub fn open(&mut self, path: &Path, action: Action) -> Result<DocumentId, Error> {
pub fn open(&mut self, path: &Path, action: Action) -> Result<DocumentId, DocumentOpenError> {
let path = helix_stdx::path::canonicalize(path);
let id = self.document_by_path(&path).map(|doc| doc.id);

Expand Down