Re: bin/58116: npf.conf(5) doesn't allow comments inside lists

To: gnats-admin%netbsd.org@localhost, netbsd-bugs%netbsd.org@localhost, campbell+netbsd%mumble.net@localhost
Subject: Re: bin/58116: npf.conf(5) doesn't allow comments inside lists
From: Attaullah <mdattaullahansari152%gmail.com@localhost>
Date: Tue, 18 Mar 2025 04:16:17 +0530

I apologize for the formatting issues in my previous email. It seems
the patch got corrupted due to email encoding. To avoid any issues,
I’m resending the patch as a file attachment.

On Tue, Mar 18, 2025 at 3:45 AM Attaullah
<mdattaullahansari152%gmail.com@localhost> wrote:
>
> The following reply was made to PR bin/58116; it has been noted by GNATS.
>
> From: Attaullah <mdattaullahansari152%gmail.com@localhost>
> To: gnats-bugs%netbsd.org@localhost
> Cc:
> Subject: Re: bin/58116: npf.conf(5) doesn't allow comments inside lists
> Date: Tue, 18 Mar 2025 03:43:19 +0530
>
>  On Fri Apr 05 2024, Taylor R Campbell wrote:
>  >>Number:         58116
>  >>Category:       bin
>  >>Synopsis:       npf.conf(5) doesn't allow comments inside lists
>  >>Confidential:   no
>  >>Severity:       serious
>  >>Priority:       medium
>  >>Responsible:    bin-bug-people
>  >>State:          open
>  >>Class:          sw-bug
>  >>Submitter-Id:   net
>  >>Arrival-Date:   Fri Apr 05 01:05:00 +0000 2024
>  >>Originator:     Taylor R Campbell
>  >>Release:        current, 10, 9, 8, ...
>  >>Organization:
>  >The NetPF Foundation
>  >>Environment:
>  >rainblobular and sleety
>  >>Description:
>  >Can't write this:
>  >
>  >$ports =3D {
>  >    123,    # foo
>  >    456,    # bar
>  >    789,    # baz
>  >}
>  >
>  >npf.conf treats line break like `;', and supports `\' continuation lines b=
>  ut only if there's no comment.  It also doesn't like the trailing comma, wh=
>  ich would be friendlier to M-x sort-lines.  So although the input can be br=
>  oken into lines, they can't have comments or be reliably reordered:
>  >
>  >$ports =3D {
>  >    123, \
>  >    456, \
>  >    789 \
>  >}
>  >>How-To-Repeat:
>  >
>  >>Fix:
>  >Yes, please!
>
>  I have modified the grammar to allow:
>  - Skipping any `\n` within curly braces `{}` in variable definitions.
>  - Allowing optional trailing commas.
>
>  and updated parser (`npf_parse.y`), lexer (`npf_scan.l`) and revised
>  documentation (`npf.conf.5`) accordingly.
>
>  Tested for the following cases
>  - NetBSD npf atf test suite
>  - Simple lists with comments and optional trailing commas
>  - Edge cases (e.g., empty lists, double comma)
>  - Old-style lists and continuation lines for backward compatibility.
>  - backslash continuation with and without comments.
>
>  Let me know if this approach works for you!
>
>  PATCH:
>  diff --git a/usr.sbin/npf/npfctl/npf.conf.5 b/usr.sbin/npf/npfctl/npf.conf.=
>  5
>  index ed13221d2d44..1f145217c59c 100644
>  --- a/usr.sbin/npf/npfctl/npf.conf.5
>  +++ b/usr.sbin/npf/npfctl/npf.conf.5
>  @@ -76,9 +76,14 @@ Variables are defined by assigning a value to them
>  as follows:
>   .Dl $var1 =3D 10.0.0.1
>   .Pp
>   A variable may also be defined as a set:
>  +.Bd -literal -offset indent
>  +$var2 =3D {
>  +    10.0.0.1,   # First host
>  +    10.0.0.2,   # Second host
>  +}
>  +.Ed
>   .Pp
>  -.Dl $var2 =3D { 10.0.0.1, 10.0.0.2 }
>  -.Pp
>  +Newlines within curly braces are ignored, and trailing commas are optional=
>  .
>   Common variable definitions are for IP addresses, networks, ports,
>   and interfaces.
>   .Ss Tables
>  @@ -531,6 +536,8 @@ The backslash
>   .Pq Sq \e
>   character at the end of a line marks a continuation line,
>   i.e., the next line is considered an extension of the present line.
>  +Additionally, within curly braces of variable definitions, newlines are
>  +allowed without continuation characters.
>   .Sh GRAMMAR
>   The following is a non-formal BNF-like definition of the grammar.
>   The definition is simplified and is intended to be human readable,
>  @@ -547,7 +554,7 @@ syntax        =3D var-def | set-param | alg | table-def=
>   |
>
>   var-name    =3D "$" . string
>   interface    =3D interface-name | var-name
>  -var-def        =3D var "=3D" ( var-value | "{" value *[ "," value ] "}" )
>  +var-def        =3D var "=3D" ( var-value | "{" value *[ "," value ] [ "," =
>  ] "}" )
>
>   # Parameter setting.
>   set-param    =3D "set" param-value
>  @@ -638,8 +645,15 @@ $int_if =3D { inet4(wm1) }
>   table <blocklist> type ipset file "/etc/npf_blocklist"
>   table <limited> type lpm
>
>  -$services_tcp =3D { http, https, smtp, domain, 6000, 9022 }
>  -$services_udp =3D { domain, ntp, 6000 }
>  +$services_tcp =3D {
>  +    http,    # Web traffic
>  +    https,   # Secure web traffic
>  +    smtp,    # Email sending
>  +    domain,  # DNS queries
>  +    6000,    # Custom service
>  +    9022,    # SSH forwarding
>  +}
>  +$services_udp =3D { domain, ntp, 6000, }
>   $localnet =3D { 10.1.1.0/24 }
>
>   alg "icmp"
>  diff --git a/usr.sbin/npf/npfctl/npf_parse.y b/usr.sbin/npf/npfctl/npf_pars=
>  e.y
>  index d77f462cd8c5..169c74beb28c 100644
>  --- a/usr.sbin/npf/npfctl/npf_parse.y
>  +++ b/usr.sbin/npf/npfctl/npf_parse.y
>  @@ -135,6 +135,7 @@ yyerror(const char *fmt, ...)
>   %token            IPSET
>   %token            LPM
>   %token            MAP
>  +%token            NEWLINE
>   %token            NO_PORTS
>   %token            MINUS
>   %token            NAME
>  @@ -158,7 +159,7 @@ yyerror(const char *fmt, ...)
>   %token            RETURNRST
>   %token            ROUNDROBIN
>   %token            RULESET
>  -%token            SEPLINE
>  +%token            SEMICOLON
>   %token            SET
>   %token            SLASH
>   %token            STATEFUL
>  @@ -193,7 +194,7 @@ yyerror(const char *fmt, ...)
>   %type    <var>        filt_port filt_port_list port_range icmp_type_and_co=
>  de
>   %type    <var>        filt_addr addr_and_mask tcp_flags tcp_flags_and_mask
>   %type    <var>        procs proc_call proc_param_list proc_param
>  -%type    <var>        element list_elems list value filt_addr_list
>  +%type    <var>        element list_elems list_trail list value filt_addr_l=
>  ist
>   %type    <var>        opt_proto proto proto_elems
>   %type    <addrport>    mapseg
>   %type    <filtopts>    filt_opts all_or_filt_opts
>  @@ -220,7 +221,7 @@ input
>       ;
>
>   lines
>  -    : lines SEPLINE line
>  +    : lines sepline line
>       | line
>       ;
>
>  @@ -242,6 +243,11 @@ alg
>       }
>       ;
>
>  +sepline
>  +    : NEWLINE
>  +    | SEMICOLON
>  +    ;
>  +
>   param_val
>       : number    { $$ =3D $1; }
>       | ON        { $$ =3D true; }
>  @@ -272,18 +278,17 @@ value
>       ;
>
>   list
>  -    : CURLY_OPEN list_elems CURLY_CLOSE
>  +    : CURLY_OPEN opt_nl list_elems CURLY_CLOSE
>       {
>  -        $$ =3D $2;
>  +        $$ =3D $3;
>       }
>       ;
>
>   list_elems
>  -    : list_elems COMMA element
>  +    : element list_trail
>       {
>  -        npfvar_add_elements($1, $3);
>  +        $$ =3D npfvar_add_elements($1, $2);
>       }
>  -    | element
>       ;
>
>   element
>  @@ -313,6 +318,24 @@ element
>       | addr_and_mask        { $$ =3D $1; }
>       ;
>
>  +list_trail
>  +    : element_sep element list_trail
>  +    {
>  +        $$ =3D npfvar_add_elements($2, $3);
>  +    }
>  +    | opt_nl         { $$ =3D NULL; }
>  +    | element_sep         { $$ =3D NULL; }
>  +    ;
>  +
>  +element_sep
>  +    : opt_nl COMMA opt_nl
>  +    ;
>  +
>  +opt_nl
>  +    : opt_nl NEWLINE
>  +    |
>  +    ;
>  +
>   /*
>    * Table definition.
>    */
>  @@ -430,7 +453,7 @@ rproc
>       ;
>
>   procs
>  -    : procs SEPLINE proc_call
>  +    : procs sepline proc_call
>       {
>           $$ =3D npfvar_add_elements($1, $3);
>       }
>  @@ -531,7 +554,7 @@ ruleset_block
>       ;
>
>   ruleset_def
>  -    : ruleset_def SEPLINE rule_group
>  +    : ruleset_def sepline rule_group
>       | rule_group
>       ;
>
>  diff --git a/usr.sbin/npf/npfctl/npf_scan.l b/usr.sbin/npf/npfctl/npf_scan.=
>  l
>  index 28bc5e966574..7dd6b70c8234 100644
>  --- a/usr.sbin/npf/npfctl/npf_scan.l
>  +++ b/usr.sbin/npf/npfctl/npf_scan.l
>  @@ -133,8 +133,8 @@ npt66            return NPT66;
>   "-"            return MINUS;
>   procedure        return PROCEDURE;
>   \\\n            yylineno++; yycolumn =3D 0;
>  -\n            yylineno++; yycolumn =3D 0; return SEPLINE;
>  -;            return SEPLINE;
>  +\n            yylineno++; yycolumn =3D 0; return NEWLINE;
>  +;            return SEMICOLON;
>   name            return NAME;
>   group            return GROUP;
>   default            return DEFAULT;
>

diff --git a/usr.sbin/npf/npfctl/npf.conf.5 b/usr.sbin/npf/npfctl/npf.conf.5
index ed13221d2d44..1f145217c59c 100644
--- a/usr.sbin/npf/npfctl/npf.conf.5
+++ b/usr.sbin/npf/npfctl/npf.conf.5
@@ -76,9 +76,14 @@ Variables are defined by assigning a value to them as follows:
 .Dl $var1 = 10.0.0.1
 .Pp
 A variable may also be defined as a set:
+.Bd -literal -offset indent
+$var2 = {
+    10.0.0.1,   # First host
+    10.0.0.2,   # Second host
+}
+.Ed
 .Pp
-.Dl $var2 = { 10.0.0.1, 10.0.0.2 }
-.Pp
+Newlines within curly braces are ignored, and trailing commas are optional.
 Common variable definitions are for IP addresses, networks, ports,
 and interfaces.
 .Ss Tables
@@ -531,6 +536,8 @@ The backslash
 .Pq Sq \e
 character at the end of a line marks a continuation line,
 i.e., the next line is considered an extension of the present line.
+Additionally, within curly braces of variable definitions, newlines are
+allowed without continuation characters.
 .Sh GRAMMAR
 The following is a non-formal BNF-like definition of the grammar.
 The definition is simplified and is intended to be human readable,
@@ -547,7 +554,7 @@ syntax		= var-def | set-param | alg | table-def |
 
 var-name	= "$" . string
 interface	= interface-name | var-name
-var-def		= var "=" ( var-value | "{" value *[ "," value ] "}" )
+var-def		= var "=" ( var-value | "{" value *[ "," value ] [ "," ] "}" )
 
 # Parameter setting.
 set-param	= "set" param-value
@@ -638,8 +645,15 @@ $int_if = { inet4(wm1) }
 table <blocklist> type ipset file "/etc/npf_blocklist"
 table <limited> type lpm
 
-$services_tcp = { http, https, smtp, domain, 6000, 9022 }
-$services_udp = { domain, ntp, 6000 }
+$services_tcp = {
+	http,    # Web traffic
+	https,   # Secure web traffic
+	smtp,    # Email sending
+	domain,  # DNS queries
+	6000,    # Custom service
+	9022,    # SSH forwarding
+}
+$services_udp = { domain, ntp, 6000, }
 $localnet = { 10.1.1.0/24 }
 
 alg "icmp"
diff --git a/usr.sbin/npf/npfctl/npf_parse.y b/usr.sbin/npf/npfctl/npf_parse.y
index d77f462cd8c5..169c74beb28c 100644
--- a/usr.sbin/npf/npfctl/npf_parse.y
+++ b/usr.sbin/npf/npfctl/npf_parse.y
@@ -135,6 +135,7 @@ yyerror(const char *fmt, ...)
 %token			IPSET
 %token			LPM
 %token			MAP
+%token			NEWLINE
 %token			NO_PORTS
 %token			MINUS
 %token			NAME
@@ -158,7 +159,7 @@ yyerror(const char *fmt, ...)
 %token			RETURNRST
 %token			ROUNDROBIN
 %token			RULESET
-%token			SEPLINE
+%token			SEMICOLON
 %token			SET
 %token			SLASH
 %token			STATEFUL
@@ -193,7 +194,7 @@ yyerror(const char *fmt, ...)
 %type	<var>		filt_port filt_port_list port_range icmp_type_and_code
 %type	<var>		filt_addr addr_and_mask tcp_flags tcp_flags_and_mask
 %type	<var>		procs proc_call proc_param_list proc_param
-%type	<var>		element list_elems list value filt_addr_list
+%type	<var>		element list_elems list_trail list value filt_addr_list
 %type	<var>		opt_proto proto proto_elems
 %type	<addrport>	mapseg
 %type	<filtopts>	filt_opts all_or_filt_opts
@@ -220,7 +221,7 @@ input
 	;
 
 lines
-	: lines SEPLINE line
+	: lines sepline line
 	| line
 	;
 
@@ -242,6 +243,11 @@ alg
 	}
 	;
 
+sepline
+	: NEWLINE
+	| SEMICOLON
+	;
+
 param_val
 	: number	{ $$ = $1; }
 	| ON		{ $$ = true; }
@@ -272,18 +278,17 @@ value
 	;
 
 list
-	: CURLY_OPEN list_elems CURLY_CLOSE
+	: CURLY_OPEN opt_nl list_elems CURLY_CLOSE
 	{
-		$$ = $2;
+		$$ = $3;
 	}
 	;
 
 list_elems
-	: list_elems COMMA element
+	: element list_trail
 	{
-		npfvar_add_elements($1, $3);
+		$$ = npfvar_add_elements($1, $2);
 	}
-	| element
 	;
 
 element
@@ -313,6 +318,24 @@ element
 	| addr_and_mask		{ $$ = $1; }
 	;
 
+list_trail
+	: element_sep element list_trail
+	{
+		$$ = npfvar_add_elements($2, $3);
+	}
+	| opt_nl 		{ $$ = NULL; }
+	| element_sep 		{ $$ = NULL; }
+	;
+
+element_sep
+	: opt_nl COMMA opt_nl
+	;
+
+opt_nl
+	: opt_nl NEWLINE
+	|
+	;
+
 /*
  * Table definition.
  */
@@ -430,7 +453,7 @@ rproc
 	;
 
 procs
-	: procs SEPLINE proc_call
+	: procs sepline proc_call
 	{
 		$$ = npfvar_add_elements($1, $3);
 	}
@@ -531,7 +554,7 @@ ruleset_block
 	;
 
 ruleset_def
-	: ruleset_def SEPLINE rule_group
+	: ruleset_def sepline rule_group
 	| rule_group
 	;
 
diff --git a/usr.sbin/npf/npfctl/npf_scan.l b/usr.sbin/npf/npfctl/npf_scan.l
index 28bc5e966574..7dd6b70c8234 100644
--- a/usr.sbin/npf/npfctl/npf_scan.l
+++ b/usr.sbin/npf/npfctl/npf_scan.l
@@ -133,8 +133,8 @@ npt66			return NPT66;
 "-"			return MINUS;
 procedure		return PROCEDURE;
 \\\n			yylineno++; yycolumn = 0;
-\n			yylineno++; yycolumn = 0; return SEPLINE;
-;			return SEPLINE;
+\n			yylineno++; yycolumn = 0; return NEWLINE;
+;			return SEMICOLON;
 name			return NAME;
 group			return GROUP;
 default			return DEFAULT;

References:
- Re: bin/58116: npf.conf(5) doesn't allow comments inside lists
  - From: Attaullah

Prev by Date: misc/59194: option 2 on evbarm boot menu doesn't work
Next by Date: Re: kern/59165: vio9p(4): expose tag through sysctl or device properties
Previous by Thread: Re: bin/58116: npf.conf(5) doesn't allow comments inside lists
Next by Thread: Re: lib/57214 (The return value of pthread_sigmask is not standards compliant)
Indexes:

Home | Main Index | Thread Index | Old Index