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

[Attaullah <mdattaullahansari152%gmail.com@localhost>]

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;