Skip to content

Commit 6a914d7

Browse files
squellsquell
authored
Report error location for include errors (#971)
2 parents f17dda8 + 9890075 commit 6a914d7

File tree

7 files changed

+87
-52
lines changed

7 files changed

+87
-52
lines changed

‎src/sudo/diagnostic.rs

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
use std::fs::File;
22
use std::io::{BufRead, BufReader};
3-
use std::ops::Range;
43
use std::path::Path;
54

6-
pub(crate) fn cited_error(message: &str, range: Range<(usize, usize)>, path: impl AsRef<Path>) {
5+
use crate::sudoers::Span;
6+
7+
pub(crate) fn cited_error(message: &str, span: Span, path: impl AsRef<Path>) {
78
let path_str = path.as_ref().display();
8-
let Range {
9+
let Span {
910
start: (line, col),
1011
end: (end_line, mut end_col),
11-
} = range;
12+
} = span;
1213
eprintln_ignore_io_error!("{path_str}:{line}:{col}: {message}");
1314

1415
// we won't try to "span" errors across multiple lines

‎src/sudoers/ast.rs

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,8 @@ pub enum Directive {
142142
pub enum Sudo {
143143
Spec(PermissionSpec) = HARDENED_ENUM_VALUE_0,
144144
Decl(Directive) = HARDENED_ENUM_VALUE_1,
145-
Include(String) = HARDENED_ENUM_VALUE_2,
146-
IncludeDir(String) = HARDENED_ENUM_VALUE_3,
145+
Include(String, Span) = HARDENED_ENUM_VALUE_2,
146+
IncludeDir(String, Span) = HARDENED_ENUM_VALUE_3,
147147
LineComment = HARDENED_ENUM_VALUE_4,
148148
}
149149

@@ -175,7 +175,7 @@ impl Sudo {
175175

176176
#[cfg(test)]
177177
pub fn as_include(&self) -> &str {
178-
if let Self::Include(v) = self {
178+
if let Self::Include(v, _) = self {
179179
v
180180
} else {
181181
panic!()
@@ -554,11 +554,11 @@ impl Parse for Sudo {
554554

555555
/// Parse the include/include dir part that comes after the '#' or '@' prefix symbol
556556
fn parse_include(stream: &mut CharStream) -> Parsed<Sudo> {
557-
fn get_path(stream: &mut CharStream) -> Parsed<String> {
558-
if accept_if(|c| c == '"', stream).is_some() {
557+
fn get_path(stream: &mut CharStream, key_pos: (usize, usize)) -> Parsed<(String, Span)> {
558+
let path = if accept_if(|c| c == '"', stream).is_some() {
559559
let QuotedInclude(path) = expect_nonterminal(stream)?;
560560
expect_syntax('"', stream)?;
561-
make(path)
561+
path
562562
} else {
563563
let value_pos = stream.get_pos();
564564
let IncludePath(path) = expect_nonterminal(stream)?;
@@ -569,14 +569,27 @@ fn parse_include(stream: &mut CharStream) -> Parsed<Sudo> {
569569
"use quotes around filenames or escape whitespace"
570570
)
571571
}
572-
make(path)
573-
}
572+
path
573+
};
574+
make((
575+
path,
576+
Span {
577+
start: key_pos,
578+
end: stream.get_pos(),
579+
},
580+
))
574581
}
575582

576583
let key_pos = stream.get_pos();
577584
let result = match try_nonterminal(stream)? {
578-
Some(Username(key)) if key == "include" => Sudo::Include(get_path(stream)?),
579-
Some(Username(key)) if key == "includedir" => Sudo::IncludeDir(get_path(stream)?),
585+
Some(Username(key)) if key == "include" => {
586+
let (path, span) = get_path(stream, key_pos)?;
587+
Sudo::Include(path, span)
588+
}
589+
Some(Username(key)) if key == "includedir" => {
590+
let (path, span) = get_path(stream, key_pos)?;
591+
Sudo::IncludeDir(path, span)
592+
}
580593
_ => unrecoverable!(pos = key_pos, stream, "unknown directive"),
581594
};
582595

‎src/sudoers/basic_parser.rs

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,17 @@
2828
/// Type holding a parsed object (or error information if parsing failed)
2929
pub type Parsed<T> = Result<T, Status>;
3030

31-
pub type Position = std::ops::Range<(usize, usize)>;
31+
#[derive(Copy, Clone)]
32+
#[cfg_attr(test, derive(Debug, PartialEq))]
33+
pub struct Span {
34+
pub start: (usize, usize),
35+
pub end: (usize, usize),
36+
}
3237

3338
#[cfg_attr(test, derive(Debug, PartialEq))]
3439
pub enum Status {
35-
Fatal(Position, String), // not recoverable; stream in inconsistent state
36-
Reject, // parsing failed by no input consumed
40+
Fatal(Span, String), // not recoverable; stream in inconsistent state
41+
Reject, // parsing failed by no input consumed
3742
}
3843

3944
pub fn make<T>(value: T) -> Parsed<T> {
@@ -46,14 +51,12 @@ pub fn reject<T>() -> Parsed<T> {
4651

4752
macro_rules! unrecoverable {
4853
(pos=$pos:expr, $stream:ident, $($str:expr),*) => {
49-
return Err(crate::sudoers::basic_parser::Status::Fatal($pos .. CharStream::get_pos($stream), format![$($str),*]))
50-
};
51-
($stream:ident, $($str:expr),*) => {
52-
{
53-
let pos = CharStream::get_pos($stream);
54-
return Err(crate::sudoers::basic_parser::Status::Fatal(pos .. pos, format![$($str),*]))
55-
}
54+
return Err(crate::sudoers::basic_parser::Status::Fatal(Span { start: $pos, end: CharStream::get_pos($stream)}, format![$($str),*]))
5655
};
56+
($stream:ident, $($str:expr),*) => {{
57+
let pos = CharStream::get_pos($stream);
58+
return Err(crate::sudoers::basic_parser::Status::Fatal(Span { start: pos, end: pos }, format![$($str),*]))
59+
}};
5760
($($str:expr),*) => {
5861
return Err(crate::basic_parser::Status::Fatal(Default::default(), format![$($str),*]))
5962
};

‎src/sudoers/mod.rs

Lines changed: 25 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,15 @@ use ast::*;
2222
use tokens::*;
2323

2424
pub type Settings = defaults::Settings;
25+
pub use basic_parser::Span;
2526

2627
/// How many nested include files do we allow?
2728
const INCLUDE_LIMIT: u8 = 128;
2829

2930
/// Export some necessary symbols from modules
3031
pub struct Error {
3132
pub source: Option<PathBuf>,
32-
pub location: Option<basic_parser::Position>,
33+
pub location: Option<basic_parser::Span>,
3334
pub message: String,
3435
}
3536

@@ -548,14 +549,15 @@ fn analyze(
548549
fn include(
549550
cfg: &mut Sudoers,
550551
parent: &Path,
552+
span: Span,
551553
path: &Path,
552554
diagnostics: &mut Vec<Error>,
553555
count: &mut u8,
554556
) {
555557
if *count >= INCLUDE_LIMIT {
556558
diagnostics.push(Error {
557559
source: Some(parent.to_owned()),
558-
location: None,
560+
location: Some(span),
559561
message: format!("include file limit reached opening '{}'", path.display()),
560562
})
561563
// FIXME: this will cause an error in `visudo` if we open a non-privileged sudoers file
@@ -576,7 +578,7 @@ fn analyze(
576578

577579
diagnostics.push(Error {
578580
source: Some(parent.to_owned()),
579-
location: None,
581+
location: Some(span),
580582
message,
581583
})
582584
}
@@ -609,28 +611,33 @@ fn analyze(
609611
}
610612
}
611613

612-
Sudo::Include(path) => include(
614+
Sudo::Include(path, span) => include(
613615
cfg,
614616
cur_path,
617+
span,
615618
&resolve_relative(cur_path, path),
616619
diagnostics,
617620
safety_count,
618621
),
619622

620-
Sudo::IncludeDir(path) => {
623+
Sudo::IncludeDir(path, span) => {
621624
if path.contains("%h") {
622-
diagnostics.push(Error{
623-
source: Some(cur_path.to_owned()),
624-
location: None,
625-
message: format!("cannot open sudoers file {path}: percent escape %h in includedir is unsupported")});
625+
diagnostics.push(Error {
626+
source: Some(cur_path.to_owned()),
627+
location: Some(span),
628+
message: format!(
629+
"cannot open sudoers file {path}: \
630+
percent escape %h in includedir is unsupported"
631+
),
632+
});
626633
continue;
627634
}
628635

629636
let path = resolve_relative(cur_path, path);
630637
let Ok(files) = std::fs::read_dir(&path) else {
631638
diagnostics.push(Error {
632639
source: Some(cur_path.to_owned()),
633-
location: None,
640+
location: Some(span),
634641
message: format!("cannot open sudoers file {}", path.display()),
635642
});
636643
continue;
@@ -648,7 +655,14 @@ fn analyze(
648655
.collect::<Vec<_>>();
649656
safe_files.sort();
650657
for file in safe_files {
651-
include(cfg, cur_path, file.as_ref(), diagnostics, safety_count)
658+
include(
659+
cfg,
660+
cur_path,
661+
span,
662+
file.as_ref(),
663+
diagnostics,
664+
safety_count,
665+
)
652666
}
653667
}
654668
},

‎src/sudoers/test/mod.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,14 @@ fn gh676_percent_h_escape_unsupported() {
386386
assert_eq!(
387387
errs[0].message,
388388
"cannot open sudoers file /etc/%h: percent escape %h in includedir is unsupported"
389-
)
389+
);
390+
assert_eq!(
391+
errs[0].location,
392+
Some(Span {
393+
start: (1, 2),
394+
end: (1, 23)
395+
})
396+
);
390397
}
391398

392399
#[test]

‎test-framework/sudo-compliance-tests/src/sudo/sudoers/include.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ fn file_does_not_exist() -> Result<()> {
3737
let diagnostic = if sudo_test::is_original_sudo() {
3838
"sudo: unable to open /etc/sudoers2: No such file or directory"
3939
} else {
40-
"sudo-rs: cannot open sudoers file '/etc/sudoers2'"
40+
"cannot open sudoers file '/etc/sudoers2'"
4141
};
4242
assert_contains!(output.stderr(), diagnostic);
4343
Ok(())
@@ -128,7 +128,7 @@ fn include_loop_error_messages() -> Result<()> {
128128
let diagnostic = if sudo_test::is_original_sudo() {
129129
"/etc/sudoers2: too many levels of includes"
130130
} else {
131-
"sudo-rs: include file limit reached opening '/etc/sudoers2'"
131+
"include file limit reached opening '/etc/sudoers2'"
132132
};
133133
assert_contains!(output.stderr(), diagnostic);
134134

@@ -147,7 +147,7 @@ fn include_loop_not_fatal() -> Result<()> {
147147
let diagnostic = if sudo_test::is_original_sudo() {
148148
"/etc/sudoers2: too many levels of includes"
149149
} else {
150-
"sudo-rs: include file limit reached opening '/etc/sudoers2'"
150+
"include file limit reached opening '/etc/sudoers2'"
151151
};
152152
assert_contains!(output.stderr(), diagnostic);
153153

@@ -170,7 +170,7 @@ fn permissions_check() -> Result<()> {
170170
let diagnostic = if sudo_test::is_original_sudo() {
171171
"sudo: /etc/sudoers2 is world writable"
172172
} else {
173-
"sudo-rs: /etc/sudoers2 cannot be world-writable"
173+
"/etc/sudoers2 cannot be world-writable"
174174
};
175175
assert_contains!(output.stderr(), diagnostic);
176176

@@ -189,7 +189,7 @@ fn permissions_check_not_fatal() -> Result<()> {
189189
let diagnostic = if sudo_test::is_original_sudo() {
190190
format!("sudo: {ETC_DIR}/sudoers2 is world writable")
191191
} else {
192-
format!("sudo-rs: {ETC_DIR}/sudoers2 cannot be world-writable")
192+
format!("{ETC_DIR}/sudoers2 cannot be world-writable")
193193
};
194194
assert_contains!(output.stderr(), diagnostic);
195195

@@ -213,7 +213,7 @@ fn ownership_check() -> Result<()> {
213213
let diagnostic = if sudo_test::is_original_sudo() {
214214
"sudo: /etc/sudoers2 is owned by uid 1000, should be 0"
215215
} else {
216-
"sudo-rs: /etc/sudoers2 must be owned by root"
216+
"/etc/sudoers2 must be owned by root"
217217
};
218218
assert_contains!(output.stderr(), diagnostic);
219219

@@ -233,7 +233,7 @@ fn ownership_check_not_fatal() -> Result<()> {
233233
let diagnostic = if sudo_test::is_original_sudo() {
234234
"sudo: /etc/sudoers2 is owned by uid 1000, should be 0"
235235
} else {
236-
"sudo-rs: /etc/sudoers2 must be owned by root"
236+
"/etc/sudoers2 must be owned by root"
237237
};
238238
assert_contains!(output.stderr(), diagnostic);
239239

@@ -284,7 +284,7 @@ fn relative_path_grandparent_directory() -> Result<()> {
284284
let diagnostic = if sudo_test::is_original_sudo() {
285285
format!("sudo: unable to open {path}: No such file or directory")
286286
} else {
287-
format!("sudo-rs: cannot open sudoers file '{path}'")
287+
format!("cannot open sudoers file '{path}'")
288288
};
289289
assert_contains!(output.stderr(), diagnostic);
290290
Ok(())

‎test-framework/sudo-compliance-tests/src/sudo/sudoers/includedir.rs

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -81,10 +81,7 @@ fn directory_does_not_exist_is_not_fatal() -> Result<()> {
8181
if sudo_test::is_original_sudo() {
8282
assert!(stderr.is_empty());
8383
} else {
84-
assert_contains!(
85-
stderr,
86-
"sudo-rs: cannot open sudoers file /etc/does-not-exist"
87-
);
84+
assert_contains!(stderr, "cannot open sudoers file /etc/does-not-exist");
8885
}
8986

9087
Ok(())
@@ -192,7 +189,7 @@ fn include_loop() -> Result<()> {
192189
let diagnostic = if sudo_test::is_original_sudo() {
193190
format!("{ETC_DIR}/sudoers.d/a: too many levels of includes")
194191
} else {
195-
format!("sudo-rs: include file limit reached opening '{ETC_DIR}/sudoers.d/a'")
192+
format!("include file limit reached opening '{ETC_DIR}/sudoers.d/a'")
196193
};
197194
assert_contains!(output.stderr(), diagnostic);
198195

@@ -227,7 +224,7 @@ fn statements_prior_to_include_loop_are_evaluated() -> Result<()> {
227224
let diagnostic = if sudo_test::is_original_sudo() {
228225
format!("{ETC_DIR}/sudoers.d/a: too many levels of includes")
229226
} else {
230-
format!("sudo-rs: include file limit reached opening '{ETC_DIR}/sudoers.d/a'")
227+
format!("include file limit reached opening '{ETC_DIR}/sudoers.d/a'")
231228
};
232229

233230
assert_contains!(output.stderr(), diagnostic);
@@ -350,7 +347,7 @@ fn ignores_directory_with_bad_perms() -> Result<()> {
350347
]
351348
} else {
352349
[
353-
format!("sudo-rs: {ETC_DIR}/sudoers2.d cannot be world-writable"),
350+
format!("{ETC_DIR}/sudoers2.d cannot be world-writable"),
354351
"I'm sorry root. I'm afraid I can't do that".to_owned(),
355352
]
356353
};
@@ -380,7 +377,7 @@ fn ignores_directory_with_bad_ownership() -> Result<()> {
380377
]
381378
} else {
382379
[
383-
format!("sudo-rs: {ETC_DIR}/sudoers2.d must be owned by root"),
380+
format!("{ETC_DIR}/sudoers2.d must be owned by root"),
384381
"I'm sorry root. I'm afraid I can't do that".to_owned(),
385382
]
386383
};

0 commit comments

Comments
 (0)