Henning Brauer(hb-openbsdt...@ml.bsws.de) on 2016.06.21 13:11:16 +0200: > * Stefan Sperling <s...@stsp.name> [2016-06-21 11:15]: > > Generally, I would appreciate more detailed error messages from the pf.conf > > parser. I recall several occasions where pfctl threw "syntax error" and more > > specific error reporting would have saved me some time with finding the > > silly mistake I made. And in this case the ruleset loads successfully even > > though, while parsing, we already know it's not going to work as intended... > > true, that's shared by all yacc-style parsers, if the grammar doesn't > match you just get syntax error without much of a hint what's wrong.
the quality of the error message depends both on the language (some make it harder to error out early) and on the parser. fail early is better. > however, the ruleset in this case does NOT load. > > <brahe@quigon> $ echo '"a macro with spaces"="foo"\npass from $a\ macro\ > with\ spaces"' | pfctl -nvf - > a macro with spaces = "foo" > stdin:2: macro 'a' not defined > stdin:2: syntax error > sure, thats what the op reported > > > Only as long as it doesn't make the parser code overly complex, of course. > > But currently the balance is tilted too much towards terse error messages > > for my taste. So I liked benno's first diff. > > it's just a tiny check indeed, which swings the "cost" (not in > financial terms) vs benefit pendulum towards the benefit side, yes. thx. here is a diff for all daemons. ok for this? diff --git sbin/iked/parse.y sbin/iked/parse.y index 958e51a..6455a00 100644 --- sbin/iked/parse.y +++ sbin/iked/parse.y @@ -73,6 +73,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -1006,6 +1007,10 @@ string : string STRING varset : STRING '=' string { log_debug("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) err(1, "cannot store variable"); free($1); @@ -1234,6 +1239,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { unsigned char buf[8096]; diff --git sbin/ipsecctl/parse.y sbin/ipsecctl/parse.y index fe9cee0..67fbd72 100644 --- sbin/ipsecctl/parse.y +++ sbin/ipsecctl/parse.y @@ -71,6 +71,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -883,6 +884,10 @@ varset : STRING '=' string { if (ipsec->opts & IPSECCTL_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) err(1, "cannot store variable"); free($1); @@ -1092,6 +1097,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git sbin/pfctl/parse.y sbin/pfctl/parse.y index 934438c..9846063 100644 --- sbin/pfctl/parse.y +++ sbin/pfctl/parse.y @@ -85,6 +85,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -714,6 +715,10 @@ numberstring : NUMBER { varset : STRING '=' varstring { if (pf->opts & PF_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) err(1, "cannot store variable %s", $1); free($1); @@ -5172,6 +5177,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/bgpd/parse.y usr.sbin/bgpd/parse.y index e7fd5e1..93c847c 100644 --- usr.sbin/bgpd/parse.y +++ usr.sbin/bgpd/parse.y @@ -64,6 +64,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -339,6 +340,10 @@ yesno : STRING { varset : STRING '=' string { if (cmd_opts & BGPD_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -2406,6 +2411,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/dvmrpd/parse.y usr.sbin/dvmrpd/parse.y index 66b7b73..66aa47f 100644 --- usr.sbin/dvmrpd/parse.y +++ usr.sbin/dvmrpd/parse.y @@ -66,6 +66,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -165,6 +166,10 @@ yesno : STRING { varset : STRING '=' string { if (conf->opts & DVMRPD_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -504,6 +509,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/eigrpd/parse.y usr.sbin/eigrpd/parse.y index 02e010f..09067fb 100644 --- usr.sbin/eigrpd/parse.y +++ usr.sbin/eigrpd/parse.y @@ -65,6 +65,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -199,6 +200,10 @@ eigrp_af : IPV4 { $$ = AF_INET; } varset : STRING '=' string { if (global.cmd_opts & EIGRPD_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -731,6 +736,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { unsigned char buf[8096]; diff --git usr.sbin/hostapd/parse.y usr.sbin/hostapd/parse.y index c6a8beb..56c6bda 100644 --- usr.sbin/hostapd/parse.y +++ usr.sbin/hostapd/parse.y @@ -78,6 +78,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -971,6 +972,10 @@ string : string STRING varset : STRING '=' string { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) hostapd_fatal("cannot store variable"); free($1); @@ -1420,6 +1425,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/httpd/parse.y usr.sbin/httpd/parse.y index 080f319..bce7741 100644 --- usr.sbin/httpd/parse.y +++ usr.sbin/httpd/parse.y @@ -75,6 +75,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -171,6 +172,10 @@ include : INCLUDE STRING { ; varset : STRING '=' STRING { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -1305,6 +1310,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { unsigned char buf[8096]; diff --git usr.sbin/ifstated/parse.y usr.sbin/ifstated/parse.y index 5679164..a669149 100644 --- usr.sbin/ifstated/parse.y +++ usr.sbin/ifstated/parse.y @@ -63,6 +63,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -144,6 +145,10 @@ string : string STRING { varset : STRING '=' string { if (conf->opts & IFSD_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) { free($1); free($3); @@ -502,6 +507,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/iscsictl/parse.y usr.sbin/iscsictl/parse.y index b39b2e1..789f6ad 100644 --- usr.sbin/iscsictl/parse.y +++ usr.sbin/iscsictl/parse.y @@ -65,6 +65,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); void clear_config(struct iscsi_config *); @@ -155,6 +156,10 @@ string : string STRING { ; varset : STRING '=' string { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) err(1, "cannot store variable"); free($1); @@ -478,6 +483,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/ldapd/parse.y usr.sbin/ldapd/parse.y index aec1d12..eda1c1b 100644 --- usr.sbin/ldapd/parse.y +++ usr.sbin/ldapd/parse.y @@ -67,6 +67,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); struct listener *host_unix(const char *path); struct listener *host_v4(const char *, in_port_t); @@ -359,6 +360,10 @@ include : INCLUDE STRING { ; varset : STRING '=' STRING { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -556,6 +561,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[4096]; diff --git usr.sbin/ldpd/parse.y usr.sbin/ldpd/parse.y index d332b9e..d94640b 100644 --- usr.sbin/ldpd/parse.y +++ usr.sbin/ldpd/parse.y @@ -86,6 +86,7 @@ static int lookup(char *); static int lgetc(int); static int lungetc(int); static int findeol(void); +static int has_whitespace(const char *); static int yylex(void); static int check_file_secrecy(int, const char *); static struct file *pushfile(const char *, int); @@ -210,6 +211,10 @@ pw_type : ETHERNET { $$ = PW_TYPE_ETHERNET; } varset : STRING '=' string { if (global.cmd_opts & LDPD_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -927,6 +932,16 @@ findeol(void) } static int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +static int yylex(void) { unsigned char buf[8096]; diff --git usr.sbin/ospf6d/parse.y usr.sbin/ospf6d/parse.y index 70e7d3e..7a1aeed 100644 --- usr.sbin/ospf6d/parse.y +++ usr.sbin/ospf6d/parse.y @@ -66,6 +66,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -181,6 +182,10 @@ no : /* empty */ { $$ = 0; } varset : STRING '=' string { if (conf->opts & OSPFD_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -676,6 +681,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/ospfd/parse.y usr.sbin/ospfd/parse.y index 9b886ad..3c0c8a1 100644 --- usr.sbin/ospfd/parse.y +++ usr.sbin/ospfd/parse.y @@ -64,6 +64,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -197,6 +198,10 @@ msec : MSEC NUMBER { varset : STRING '=' string { if (conf->opts & OSPFD_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -862,6 +867,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/relayd/parse.y usr.sbin/relayd/parse.y index a6bf390..dbd5cf2 100644 --- usr.sbin/relayd/parse.y +++ usr.sbin/relayd/parse.y @@ -77,6 +77,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -349,6 +350,10 @@ port : PORT STRING { ; varset : STRING '=' STRING { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -2367,6 +2372,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/ripd/parse.y usr.sbin/ripd/parse.y index ddaca13..b4e4ec9 100644 --- usr.sbin/ripd/parse.y +++ usr.sbin/ripd/parse.y @@ -64,6 +64,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -149,6 +150,10 @@ no : /* empty */ { $$ = 0; } varset : STRING '=' string { if (conf->opts & RIPD_OPT_VERBOSE) printf("%s = \"%s\"\n", $1, $3); + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -529,6 +534,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/smtpd/parse.y usr.sbin/smtpd/parse.y index ab02719..8603a18 100644 --- usr.sbin/smtpd/parse.y +++ usr.sbin/smtpd/parse.y @@ -76,6 +76,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); int yyerror(const char *, ...) __attribute__((__format__ (printf, 1, 2))) __attribute__((__nonnull__ (1))); @@ -213,6 +214,10 @@ include : INCLUDE STRING { ; varset : STRING '=' STRING { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -1630,6 +1635,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { unsigned char buf[8096]; diff --git usr.sbin/snmpd/parse.y usr.sbin/snmpd/parse.y index 5e288ee..8e953a0 100644 --- usr.sbin/snmpd/parse.y +++ usr.sbin/snmpd/parse.y @@ -77,6 +77,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -165,6 +166,10 @@ include : INCLUDE STRING { ; varset : STRING '=' STRING { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -746,6 +751,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/vmd/parse.y usr.sbin/vmd/parse.y index 23cea73..21fa932 100644 --- usr.sbin/vmd/parse.y +++ usr.sbin/vmd/parse.y @@ -63,6 +63,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -129,6 +130,10 @@ include : INCLUDE string { ; varset : STRING '=' STRING { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatalx("cannot store variable"); free($1); @@ -423,6 +428,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096]; diff --git usr.sbin/ypldap/parse.y usr.sbin/ypldap/parse.y index 39be696..8652441 100644 --- usr.sbin/ypldap/parse.y +++ usr.sbin/ypldap/parse.y @@ -72,6 +72,7 @@ int lookup(char *); int lgetc(int); int lungetc(int); int findeol(void); +int has_whitespace(const char *); TAILQ_HEAD(symhead, sym) symhead = TAILQ_HEAD_INITIALIZER(symhead); struct sym { @@ -142,6 +143,10 @@ include : INCLUDE STRING { ; varset : STRING '=' STRING { + if (has_whitespace($1)) { + yyerror("macro name cannot contain whitespace"); + YYERROR; + } if (symset($1, $3, 0) == -1) fatal("cannot store variable"); free($1); @@ -497,6 +502,16 @@ findeol(void) } int +has_whitespace(const char *s) { + while (*s != '\0') { + if (isspace((unsigned char)*s)) + return 1; + s++; + } + return 0; +} + +int yylex(void) { u_char buf[8096];