* [pve-devel] [PATCH v2 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs
@ 2025-12-15 12:38 Robert Obkircher
2026-04-30 12:32 ` Robert Obkircher
0 siblings, 1 reply; 2+ messages in thread
From: Robert Obkircher @ 2025-12-15 12:38 UTC (permalink / raw)
To: pve-devel
Match the behavior of the corresponding perl code in parse_fw_rule and
treat everything after the first '#' on a line as the comment. This
works because validation prevents fields like the interface name from
contianing that symbol.
This fixes a bug where the firewall wouldn't start if a comment on a
rule contained a number sign.
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
Change since v1: removed unrelated newline introduced by cargo fmt.
proxmox-ve-config/src/firewall/cluster.rs | 63 ++++++++++++++++++++
proxmox-ve-config/src/firewall/types/rule.rs | 8 ++-
2 files changed, 68 insertions(+), 3 deletions(-)
diff --git a/proxmox-ve-config/src/firewall/cluster.rs b/proxmox-ve-config/src/firewall/cluster.rs
index 69d3bcd..697f8c1 100644
--- a/proxmox-ve-config/src/firewall/cluster.rs
+++ b/proxmox-ve-config/src/firewall/cluster.rs
@@ -394,4 +394,67 @@ IN BGP(REJECT) -log crit -source 1.2.3.4
assert!(empty_config.config.ipsets.is_empty());
assert!(empty_config.config.groups.is_empty());
}
+
+ #[test]
+ fn test_parse_config_comments() {
+ const CONFIG: &str = r#"
+# ignore lines that start with a # symbol
+[OPTIONS]
+[ALIASES]
+
+alias1 192.168.0.1 # comment # on alias1
+
+[IPSET ipset1] # comment # on ipset1
+
+dc/alias1 # comment # on ipset1 #1
+
+[RULES]
+
+GROUP testgroup # comment # on rule #1
+IN ACCEPT -p udp -dport 1234 -log nolog # comment # on rule #2
+
+[group testgroup] # comment # on testgroup
+
+IN ACCEPT -source dc/alias1 -p tcp -sport 1111 -log nolog # comment # on testgroup #1
+
+"#;
+
+ let mut config = CONFIG.as_bytes();
+ let config = Config::parse(&mut config).unwrap();
+
+ assert_eq!(config.config.aliases.len(), 1);
+ assert_eq!(
+ config.config.aliases["alias1"].comment(),
+ Some("comment # on alias1")
+ );
+
+ assert_eq!(config.config.ipsets.len(), 1);
+
+ let ipset1 = &config.config.ipsets["ipset1"];
+ assert_eq!(ipset1.comment.as_deref(), Some("comment # on ipset1"));
+
+ assert_eq!(ipset1.len(), 1);
+ assert_eq!(ipset1[0].comment.as_deref(), Some("comment # on ipset1 #1"));
+
+ assert_eq!(config.config.rules.len(), 2);
+ assert_eq!(
+ config.config.rules[0].comment.as_deref(),
+ Some("comment # on rule #1")
+ );
+ assert_eq!(
+ config.config.rules[1].comment.as_deref(),
+ Some("comment # on rule #2")
+ );
+
+ assert_eq!(config.config.groups.len(), 1);
+
+ let entry = &config.config.groups["testgroup"];
+ assert_eq!(entry.comment(), Some("comment # on testgroup"));
+
+ assert_eq!(entry.rules().len(), 1);
+ assert_eq!(
+ entry.rules()[0].comment.as_deref(),
+ Some("comment # on testgroup #1")
+ );
+ }
}
diff --git a/proxmox-ve-config/src/firewall/types/rule.rs b/proxmox-ve-config/src/firewall/types/rule.rs
index 811256c..2648fa8 100644
--- a/proxmox-ve-config/src/firewall/types/rule.rs
+++ b/proxmox-ve-config/src/firewall/types/rule.rs
@@ -115,7 +115,7 @@ impl FromStr for Rule {
bail!("rule must not contain any newlines!");
}
- let (line, comment) = match input.rsplit_once('#') {
+ let (line, comment) = match input.split_once('#') {
Some((line, comment)) if !comment.is_empty() => (line.trim(), Some(comment.trim())),
_ => (input.trim(), None),
};
@@ -261,13 +261,15 @@ mod tests {
#[test]
fn test_parse_rule() {
- let mut rule: Rule = "|GROUP tgr -i eth0 # acomm".parse().expect("valid rule");
+ let mut rule: Rule = "|GROUP tgr -i eth0 # acomm #comment"
+ .parse()
+ .expect("valid rule");
assert_eq!(
rule,
Rule {
disabled: true,
- comment: Some("acomm".to_string()),
+ comment: Some("acomm #comment".to_string()),
kind: Kind::Group(RuleGroup {
group: "tgr".to_string(),
iface: Some("eth0".to_string()),
--
2.47.3
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH v2 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs
2025-12-15 12:38 [pve-devel] [PATCH v2 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs Robert Obkircher
@ 2026-04-30 12:32 ` Robert Obkircher
0 siblings, 0 replies; 2+ messages in thread
From: Robert Obkircher @ 2026-04-30 12:32 UTC (permalink / raw)
To: pve-devel
ping
Someone ran into this again and opened a duplicate bug.
On 15.12.25 13:39, Robert Obkircher wrote:
> Match the behavior of the corresponding perl code in parse_fw_rule and
> treat everything after the first '#' on a line as the comment. This
> works because validation prevents fields like the interface name from
> contianing that symbol.
>
> This fixes a bug where the firewall wouldn't start if a comment on a
> rule contained a number sign.
>
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>
> Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
> Change since v1: removed unrelated newline introduced by cargo fmt.
>
> proxmox-ve-config/src/firewall/cluster.rs | 63 ++++++++++++++++++++
> proxmox-ve-config/src/firewall/types/rule.rs | 8 ++-
> 2 files changed, 68 insertions(+), 3 deletions(-)
>
> diff --git a/proxmox-ve-config/src/firewall/cluster.rs b/proxmox-ve-config/src/firewall/cluster.rs
> index 69d3bcd..697f8c1 100644
> --- a/proxmox-ve-config/src/firewall/cluster.rs
> +++ b/proxmox-ve-config/src/firewall/cluster.rs
> @@ -394,4 +394,67 @@ IN BGP(REJECT) -log crit -source 1.2.3.4
> assert!(empty_config.config.ipsets.is_empty());
> assert!(empty_config.config.groups.is_empty());
> }
> +
> + #[test]
> + fn test_parse_config_comments() {
> + const CONFIG: &str = r#"
> +# ignore lines that start with a # symbol
> +[OPTIONS]
> +[ALIASES]
> +
> +alias1 192.168.0.1 # comment # on alias1
> +
> +[IPSET ipset1] # comment # on ipset1
> +
> +dc/alias1 # comment # on ipset1 #1
> +
> +[RULES]
> +
> +GROUP testgroup # comment # on rule #1
> +IN ACCEPT -p udp -dport 1234 -log nolog # comment # on rule #2
> +
> +[group testgroup] # comment # on testgroup
> +
> +IN ACCEPT -source dc/alias1 -p tcp -sport 1111 -log nolog # comment # on testgroup #1
> +
> +"#;
> +
> + let mut config = CONFIG.as_bytes();
> + let config = Config::parse(&mut config).unwrap();
> +
> + assert_eq!(config.config.aliases.len(), 1);
> + assert_eq!(
> + config.config.aliases["alias1"].comment(),
> + Some("comment # on alias1")
> + );
> +
> + assert_eq!(config.config.ipsets.len(), 1);
> +
> + let ipset1 = &config.config.ipsets["ipset1"];
> + assert_eq!(ipset1.comment.as_deref(), Some("comment # on ipset1"));
> +
> + assert_eq!(ipset1.len(), 1);
> + assert_eq!(ipset1[0].comment.as_deref(), Some("comment # on ipset1 #1"));
> +
> + assert_eq!(config.config.rules.len(), 2);
> + assert_eq!(
> + config.config.rules[0].comment.as_deref(),
> + Some("comment # on rule #1")
> + );
> + assert_eq!(
> + config.config.rules[1].comment.as_deref(),
> + Some("comment # on rule #2")
> + );
> +
> + assert_eq!(config.config.groups.len(), 1);
> +
> + let entry = &config.config.groups["testgroup"];
> + assert_eq!(entry.comment(), Some("comment # on testgroup"));
> +
> + assert_eq!(entry.rules().len(), 1);
> + assert_eq!(
> + entry.rules()[0].comment.as_deref(),
> + Some("comment # on testgroup #1")
> + );
> + }
> }
> diff --git a/proxmox-ve-config/src/firewall/types/rule.rs b/proxmox-ve-config/src/firewall/types/rule.rs
> index 811256c..2648fa8 100644
> --- a/proxmox-ve-config/src/firewall/types/rule.rs
> +++ b/proxmox-ve-config/src/firewall/types/rule.rs
> @@ -115,7 +115,7 @@ impl FromStr for Rule {
> bail!("rule must not contain any newlines!");
> }
>
> - let (line, comment) = match input.rsplit_once('#') {
> + let (line, comment) = match input.split_once('#') {
> Some((line, comment)) if !comment.is_empty() => (line.trim(), Some(comment.trim())),
> _ => (input.trim(), None),
> };
> @@ -261,13 +261,15 @@ mod tests {
>
> #[test]
> fn test_parse_rule() {
> - let mut rule: Rule = "|GROUP tgr -i eth0 # acomm".parse().expect("valid rule");
> + let mut rule: Rule = "|GROUP tgr -i eth0 # acomm #comment"
> + .parse()
> + .expect("valid rule");
>
> assert_eq!(
> rule,
> Rule {
> disabled: true,
> - comment: Some("acomm".to_string()),
> + comment: Some("acomm #comment".to_string()),
> kind: Kind::Group(RuleGroup {
> group: "tgr".to_string(),
> iface: Some("eth0".to_string()),
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2026-04-30 12:33 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-15 12:38 [pve-devel] [PATCH v2 proxmox-ve-rs] fix #6399: firewall: fix parsing rule comments that contain hash signs Robert Obkircher
2026-04-30 12:32 ` Robert Obkircher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox