From 78e4dac993ad018bee98e245f6e858e18cc5db8a Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Thu, 18 May 2017 14:26:57 +0200 Subject: [PATCH 01/10] Fix some forgotten warnings --- sysdep/unix/timer.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/sysdep/unix/timer.h b/sysdep/unix/timer.h index ae5a27e8..aa3ed143 100644 --- a/sysdep/unix/timer.h +++ b/sysdep/unix/timer.h @@ -19,14 +19,14 @@ typedef struct timer { resource r; void (*hook)(struct timer *); void *data; - unsigned randomize; /* Amount of randomization */ - unsigned recurrent; /* Timer recurrence */ + uint randomize; /* Amount of randomization */ + uint recurrent; /* Timer recurrence */ node n; /* Internal link */ bird_clock_t expires; /* 0=inactive */ } timer; timer *tm_new(pool *); -void tm_start(timer *, unsigned after); +void tm_start(timer *, uint after); void tm_stop(timer *); void tm_dump_all(void); @@ -47,14 +47,14 @@ tm_remains(timer *t) } static inline void -tm_start_max(timer *t, unsigned after) +tm_start_max(timer *t, bird_clock_t after) { bird_clock_t rem = tm_remains(t); tm_start(t, (rem > after) ? rem : after); } static inline timer * -tm_new_set(pool *p, void (*hook)(struct timer *), void *data, unsigned rand, unsigned rec) +tm_new_set(pool *p, void (*hook)(struct timer *), void *data, uint rand, uint rec) { timer *t = tm_new(p); t->hook = hook; From e521150b8f6bed678527da1cf96e75026b86fe4f Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Thu, 18 May 2017 14:51:36 +0200 Subject: [PATCH 02/10] Fix VPN-RD parsing on 32-bit systems When shift count >= width of type the behavior is undefined. --- conf/cf-lex.l | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/conf/cf-lex.l b/conf/cf-lex.l index a0e3c275..a24f5ad7 100644 --- a/conf/cf-lex.l +++ b/conf/cf-lex.l @@ -124,7 +124,8 @@ include ^{WHITE}*include{WHITE}*\".*\"{WHITE}*; } {DIGIT}+:{DIGIT}+ { - unsigned long int l, len1 UNUSED, len2; + uint len1 UNUSED, len2; + u64 l; char *e; errno = 0; @@ -155,7 +156,8 @@ include ^{WHITE}*include{WHITE}*\".*\"{WHITE}*; } [02]:{DIGIT}+:{DIGIT}+ { - unsigned long int l, len1, len2; + uint len1, len2; + u64 l; char *e; if (yytext[0] == '0') From bb7aa06a48f52813a019861a0e06ce9fe4d20c4b Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Fri, 19 May 2017 00:33:52 +0200 Subject: [PATCH 03/10] Fix type mixing in flowspec formatting Variable of u64 type was passed to vararg function as uint. --- lib/flowspec.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/flowspec.c b/lib/flowspec.c index 0b863ed9..3fa6bac4 100644 --- a/lib/flowspec.c +++ b/lib/flowspec.c @@ -924,7 +924,7 @@ num_op_str(const byte *op) return NULL; } -static u64 +static uint get_value(const byte *val, u8 len) { switch (len) @@ -932,7 +932,8 @@ get_value(const byte *val, u8 len) case 1: return *val; case 2: return get_u16(val); case 4: return get_u32(val); - case 8: return get_u64(val); + // No component may have length 8 + // case 8: return get_u64(val); } return 0; @@ -974,7 +975,7 @@ net_format_flow_num(buffer *b, const byte *part) { const byte *last_op = NULL; const byte *op = part+1; - u64 val; + uint val; uint len; uint first = 1; @@ -1038,7 +1039,7 @@ static void net_format_flow_bitmask(buffer *b, const byte *part) { const byte *op = part+1; - u64 val; + uint val; uint len; uint first = 1; From 734e9fb8a933898cd3396786c06728bce6a754e5 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Tue, 23 May 2017 13:12:25 +0200 Subject: [PATCH 04/10] Minor cleanups and fixes --- conf/flowspec.Y | 26 +++++++++++++------------- lib/flowspec.c | 34 +++++++++++++--------------------- lib/flowspec.h | 14 ++++++++++++++ nest/proto.c | 2 +- proto/bfd/packets.c | 2 +- proto/rip/packets.c | 2 +- proto/rip/rip.c | 2 +- sysdep/linux/netlink.c | 4 ++-- sysdep/unix/io.c | 2 ++ 9 files changed, 48 insertions(+), 40 deletions(-) diff --git a/conf/flowspec.Y b/conf/flowspec.Y index a47d453b..8c72854c 100644 --- a/conf/flowspec.Y +++ b/conf/flowspec.Y @@ -44,19 +44,19 @@ CF_GRAMMAR /* Network Flow Specification */ flow_num_op: - TRUE { $$ = 0b000; } - | '=' { $$ = 0b001; } - | NEQ { $$ = 0b110; } - | '<' { $$ = 0b100; } - | LEQ { $$ = 0b101; } - | '>' { $$ = 0b010; } - | GEQ { $$ = 0b011; } - | FALSE { $$ = 0b111; } + TRUE { $$ = FLOW_OP_TRUE; } + | '=' { $$ = FLOW_OP_EQ; } + | NEQ { $$ = FLOW_OP_NEQ; } + | '<' { $$ = FLOW_OP_LT; } + | LEQ { $$ = FLOW_OP_LEQ; } + | '>' { $$ = FLOW_OP_GT; } + | GEQ { $$ = FLOW_OP_GEQ; } + | FALSE { $$ = FLOW_OP_FALSE; } ; flow_logic_op: - OR { $$ = 0x00; } - | AND { $$ = 0x40; } + OR { $$ = FLOW_OP_OR; } + | AND { $$ = FLOW_OP_AND; } ; flow_num_type_: @@ -97,13 +97,13 @@ flow_num_opts: flow_num_opt_ext_expr: expr { flow_check_cf_value_length(this_flow, $1); - flow_builder_add_op_val(this_flow, 0b001, $1); + flow_builder_add_op_val(this_flow, FLOW_OP_EQ, $1); } | expr DDOT expr { flow_check_cf_value_length(this_flow, $1); flow_check_cf_value_length(this_flow, $3); - flow_builder_add_op_val(this_flow, 0b011, $1); /* >= */ - flow_builder_add_op_val(this_flow, 0x40 | 0b101, $3); /* AND <= */ + flow_builder_add_op_val(this_flow, FLOW_OP_GEQ, $1); + flow_builder_add_op_val(this_flow, FLOW_OP_AND | FLOW_OP_LEQ, $3); } ; diff --git a/lib/flowspec.c b/lib/flowspec.c index 3fa6bac4..87ce0206 100644 --- a/lib/flowspec.c +++ b/lib/flowspec.c @@ -754,7 +754,7 @@ flow_builder_add_val_mask(struct flow_builder *fb, byte op, u32 value, u32 mask) if (a) { flow_builder_add_op_val(fb, op ^ 0x01, a); - op |= 0x40; + op |= FLOW_OP_AND; } if (b) @@ -897,28 +897,20 @@ flow_builder_clear(struct flow_builder *fb) */ /* Flowspec operators for [op, value]+ pairs */ -#define FLOW_TRUE 0b000 -#define FLOW_EQ 0b001 -#define FLOW_GT 0b010 -#define FLOW_GTE 0b011 -#define FLOW_LT 0b100 -#define FLOW_LTE 0b101 -#define FLOW_NEQ 0b110 -#define FLOW_FALSE 0b111 static const char * num_op_str(const byte *op) { switch (*op & 0x07) { - case FLOW_TRUE: return "true"; - case FLOW_EQ: return "="; - case FLOW_GT: return ">"; - case FLOW_GTE: return ">="; - case FLOW_LT: return "<"; - case FLOW_LTE: return "<="; - case FLOW_NEQ: return "!="; - case FLOW_FALSE: return "false"; + case FLOW_OP_TRUE: return "true"; + case FLOW_OP_EQ: return "="; + case FLOW_OP_GT: return ">"; + case FLOW_OP_GEQ: return ">="; + case FLOW_OP_LT: return "<"; + case FLOW_OP_LEQ: return "<="; + case FLOW_OP_NEQ: return "!="; + case FLOW_OP_FALSE: return "false"; } return NULL; @@ -985,8 +977,8 @@ net_format_flow_num(buffer *b, const byte *part) { /* XXX: I don't like this so complicated if-tree */ if (!isset_and(op) && - ((num_op( op) == FLOW_EQ) || (num_op( op) == FLOW_GTE)) && - ((num_op(last_op) == FLOW_EQ) || (num_op(last_op) == FLOW_LTE))) + ((num_op( op) == FLOW_OP_EQ) || (num_op( op) == FLOW_OP_GEQ)) && + ((num_op(last_op) == FLOW_OP_EQ) || (num_op(last_op) == FLOW_OP_LEQ))) { b->pos--; /* Remove last char (it is a space) */ buffer_puts(b, ","); @@ -1002,7 +994,7 @@ net_format_flow_num(buffer *b, const byte *part) val = get_value(op+1, len); if (!isset_end(op) && !isset_and(op) && isset_and(op+1+len) && - (num_op(op) == FLOW_GTE) && (num_op(op+1+len) == FLOW_LTE)) + (num_op(op) == FLOW_OP_GEQ) && (num_op(op+1+len) == FLOW_OP_LEQ)) { /* Display interval */ buffer_print(b, "%u..", val); @@ -1011,7 +1003,7 @@ net_format_flow_num(buffer *b, const byte *part) val = get_value(op+1, len); buffer_print(b, "%u", val); } - else if (num_op(op) == FLOW_EQ) + else if (num_op(op) == FLOW_OP_EQ) { buffer_print(b, "%u", val); } diff --git a/lib/flowspec.h b/lib/flowspec.h index 185d5a1c..4fe23da1 100644 --- a/lib/flowspec.h +++ b/lib/flowspec.h @@ -14,6 +14,20 @@ #include "lib/net.h" +/* Flow component operators */ +#define FLOW_OP_TRUE 0x00 /* 0b000 */ +#define FLOW_OP_EQ 0x01 /* 0b001 */ +#define FLOW_OP_GT 0x02 /* 0b010 */ +#define FLOW_OP_GEQ 0x03 /* 0b011 */ +#define FLOW_OP_LT 0x04 /* 0b100 */ +#define FLOW_OP_LEQ 0x05 /* 0b101 */ +#define FLOW_OP_NEQ 0x06 /* 0b110 */ +#define FLOW_OP_FALSE 0x07 /* 0b111 */ + +#define FLOW_OP_OR 0x00 +#define FLOW_OP_AND 0x40 + + /* Types of components in flowspec */ enum flow_type { FLOW_TYPE_DST_PREFIX = 1, diff --git a/nest/proto.c b/nest/proto.c index 3d764df0..361bb225 100644 --- a/nest/proto.c +++ b/nest/proto.c @@ -47,7 +47,7 @@ static void proto_shutdown_loop(struct timer *); static void proto_rethink_goal(struct proto *p); static char *proto_state_name(struct proto *p); static void channel_verify_limits(struct channel *c); -static void channel_reset_limit(struct channel_limit *l); +static inline void channel_reset_limit(struct channel_limit *l); static inline int proto_is_done(struct proto *p) diff --git a/proto/bfd/packets.c b/proto/bfd/packets.c index 06cde4d3..b76efda6 100644 --- a/proto/bfd/packets.c +++ b/proto/bfd/packets.c @@ -248,7 +248,7 @@ bfd_check_authentication(struct bfd_proto *p, struct bfd_session *s, struct bfd_ /* BFD CSNs are in 32-bit circular number space */ u32 csn = ntohl(auth->csn); if (s->rx_csn_known && - (((csn - s->rx_csn) > (3 * s->detect_mult)) || + (((csn - s->rx_csn) > (3 * (uint) s->detect_mult)) || (meticulous && (csn == s->rx_csn)))) { /* We want to report both new and old CSN */ diff --git a/proto/rip/packets.c b/proto/rip/packets.c index 9dc492b7..e97809c8 100644 --- a/proto/rip/packets.c +++ b/proto/rip/packets.c @@ -450,7 +450,7 @@ rip_send_response(struct rip_proto *p, struct rip_iface *ifa) /* Stale entries that should be removed */ if ((en->valid == RIP_ENTRY_STALE) && - ((en->changed + ifa->cf->garbage_time) <= now)) + ((en->changed + (bird_clock_t) ifa->cf->garbage_time) <= now)) goto next_entry; /* Triggered updates */ diff --git a/proto/rip/rip.c b/proto/rip/rip.c index 157093aa..55fb47c5 100644 --- a/proto/rip/rip.c +++ b/proto/rip/rip.c @@ -684,7 +684,7 @@ rip_reconfigure_iface(struct rip_proto *p, struct rip_iface *ifa, struct rip_ifa rip_iface_update_buffers(ifa); - if (ifa->next_regular > (now + new->update_time)) + if (ifa->next_regular > (now + (bird_clock_t) new->update_time)) ifa->next_regular = now + (random() % new->update_time) + 1; if (new->check_link != old->check_link) diff --git a/sysdep/linux/netlink.c b/sysdep/linux/netlink.c index 8f44b007..40d1196e 100644 --- a/sysdep/linux/netlink.c +++ b/sysdep/linux/netlink.c @@ -62,8 +62,8 @@ #ifndef HAVE_STRUCT_RTVIA struct rtvia { - __kernel_sa_family_t rtvia_family; - __u8 rtvia_addr[0]; + unsigned short rtvia_family; + u8 rtvia_addr[0]; }; #endif diff --git a/sysdep/unix/io.c b/sysdep/unix/io.c index d1246ea5..0cf48c9d 100644 --- a/sysdep/unix/io.c +++ b/sysdep/unix/io.c @@ -1344,6 +1344,7 @@ sk_tcp_connected(sock *s) s->tx_hook(s); } +#ifdef HAVE_LIBSSH static void sk_ssh_connected(sock *s) { @@ -1351,6 +1352,7 @@ sk_ssh_connected(sock *s) s->type = SK_SSH; s->tx_hook(s); } +#endif static int sk_passive_connected(sock *s, int type) From 0705a1c5658c2682c915007f466f55d2a8f7ec14 Mon Sep 17 00:00:00 2001 From: Pavel Tvrdik Date: Thu, 30 Jun 2016 15:04:49 +0200 Subject: [PATCH 05/10] Add a hint for an invalid IP prefix bird> eval 200.210.220.0/16 Invalid IPv4 prefix 200.210.220.0/16, maybe you wanted 200.210.0.0/16 bird> eval 1000:2000::/8 Invalid IPv6 prefix 1000:2000::/8, maybe you wanted 1000::/8 --- conf/confbase.Y | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/conf/confbase.Y b/conf/confbase.Y index 47ee07ce..e7fd1677 100644 --- a/conf/confbase.Y +++ b/conf/confbase.Y @@ -187,17 +187,24 @@ pxlen4: net_ip4_: IP4 pxlen4 { net_fill_ip4(&($$), $1, $2); - if (!net_validate_ip4((net_addr_ip4 *) &($$))) - cf_error("Invalid IPv4 prefix"); + + net_addr_ip4 *n = (void *) &($$); + if (!net_validate_ip4(n)) + cf_error("Invalid IPv4 prefix %I4/%d, maybe you wanted %I4/%d", + n->prefix, n->pxlen, ip4_and(n->prefix, ip4_mkmask(n->pxlen)), n->pxlen); }; net_ip6_: IP6 '/' NUM { - net_fill_ip6(&($$), $1, $3); if ($3 < 0 || $3 > IP6_MAX_PREFIX_LENGTH) cf_error("Invalid prefix length %d", $3); - if (!net_validate_ip6((net_addr_ip6 *) &($$))) - cf_error("Invalid IPv6 prefix"); + + net_fill_ip6(&($$), $1, $3); + + net_addr_ip6 *n = (void *) &($$); + if (!net_validate_ip6(n)) + cf_error("Invalid IPv6 prefix %I6/%d, maybe you wanted %I6/%d", + n->prefix, n->pxlen, ip6_and(n->prefix, ip6_mkmask(n->pxlen)), n->pxlen); }; net_vpn4_: VPN_RD net_ip4_ From 6aaaa63519c88c872f15dcc639643103b563b1c6 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Tue, 23 May 2017 17:22:53 +0200 Subject: [PATCH 06/10] Change parser to handle numbers as unsigned Lexer always parsed numbers as unsigned, but parser handled them as signed and grammar contained many unnecessary checks for negativity. --- conf/confbase.Y | 20 ++++++++++---------- filter/config.Y | 4 ++-- filter/filter.h | 4 ++-- nest/config.Y | 4 ++-- proto/ospf/config.Y | 12 ++++++------ proto/radv/config.Y | 20 +++++++++----------- proto/rip/config.Y | 2 +- 7 files changed, 32 insertions(+), 34 deletions(-) diff --git a/conf/confbase.Y b/conf/confbase.Y index e7fd1677..901ca2b2 100644 --- a/conf/confbase.Y +++ b/conf/confbase.Y @@ -27,16 +27,16 @@ CF_HDR CF_DEFINES static void -check_u16(unsigned val) +check_u16(uint val) { if (val > 0xFFFF) - cf_error("Value %d out of range (0-65535)", val); + cf_error("Value %u out of range (0-65535)", val); } CF_DECLS %union { - int i; + uint i; u32 i32; u64 i64; ip_addr a; @@ -175,7 +175,7 @@ ipa_scope: pxlen4: '/' NUM { - if ($2 < 0 || $2 > IP4_MAX_PREFIX_LENGTH) cf_error("Invalid prefix length %d", $2); + if ($2 > IP4_MAX_PREFIX_LENGTH) cf_error("Invalid prefix length %u", $2); $$ = $2; } | ':' IP4 { @@ -196,8 +196,8 @@ net_ip4_: IP4 pxlen4 net_ip6_: IP6 '/' NUM { - if ($3 < 0 || $3 > IP6_MAX_PREFIX_LENGTH) - cf_error("Invalid prefix length %d", $3); + if ($3 > IP6_MAX_PREFIX_LENGTH) + cf_error("Invalid prefix length %u", $3); net_fill_ip6(&($$), $1, $3); @@ -223,16 +223,16 @@ net_roa4_: net_ip4_ MAX NUM AS NUM { $$ = cfg_alloc(sizeof(net_addr_roa4)); net_fill_roa4($$, net4_prefix(&$1), net4_pxlen(&$1), $3, $5); - if ($3 < (int) net4_pxlen(&$1) || $3 > IP4_MAX_PREFIX_LENGTH) - cf_error("Invalid max prefix length %d", $3); + if ($3 < net4_pxlen(&$1) || $3 > IP4_MAX_PREFIX_LENGTH) + cf_error("Invalid max prefix length %u", $3); }; net_roa6_: net_ip6_ MAX NUM AS NUM { $$ = cfg_alloc(sizeof(net_addr_roa6)); net_fill_roa6($$, net6_prefix(&$1), net6_pxlen(&$1), $3, $5); - if ($3 < (int) net6_pxlen(&$1) || $3 > IP6_MAX_PREFIX_LENGTH) - cf_error("Invalid max prefix length %d", $3); + if ($3 < net6_pxlen(&$1) || $3 > IP6_MAX_PREFIX_LENGTH) + cf_error("Invalid max prefix length %u", $3); }; net_ip_: net_ip4_ | net_ip6_ ; diff --git a/filter/config.Y b/filter/config.Y index 2864d290..a64bb932 100644 --- a/filter/config.Y +++ b/filter/config.Y @@ -728,8 +728,8 @@ fprefix: | net_ip_ '-' { $$.net = $1; $$.lo = 0; $$.hi = $1.pxlen; } | net_ip_ '{' NUM ',' NUM '}' { $$.net = $1; $$.lo = $3; $$.hi = $5; - if ((0 > $3) || ($3 > $5) || ($5 > net_max_prefix_length[$1.type])) - cf_error("Invalid prefix pattern range: {%d, %d}", $3, $5); + if (($3 > $5) || ($5 > net_max_prefix_length[$1.type])) + cf_error("Invalid prefix pattern range: {%u, %u}", $3, $5); } ; diff --git a/filter/filter.h b/filter/filter.h index 0beac679..6c81b9bc 100644 --- a/filter/filter.h +++ b/filter/filter.h @@ -19,11 +19,11 @@ struct f_inst { /* Instruction */ u16 code; /* Instruction code, see the interpret() function and P() macro */ u16 aux; /* Extension to instruction code, T_*, EA_*, EAF_* */ union { - int i; + uint i; void *p; } a1; /* The first argument */ union { - int i; + uint i; void *p; } a2; /* The second argument */ int lineno; diff --git a/nest/config.Y b/nest/config.Y index e672d730..220fa8b0 100644 --- a/nest/config.Y +++ b/nest/config.Y @@ -349,8 +349,8 @@ iface_patt: ; tos: - CLASS expr { $$ = $2 & 0xfc; if (($2 < 0) || ($2 > 255)) cf_error("TX class must be in range 0-255"); } - | DSCP expr { $$ = ($2 & 0x3f) << 2; if (($2 < 0) || ($2 > 63)) cf_error("TX DSCP must be in range 0-63"); } + CLASS expr { $$ = $2 & 0xfc; if ($2 > 255) cf_error("TX class must be in range 0-255"); } + | DSCP expr { $$ = ($2 & 0x3f) << 2; if ($2 > 63) cf_error("TX DSCP must be in range 0-63"); } ; /* Direct device route protocol */ diff --git a/proto/ospf/config.Y b/proto/ospf/config.Y index 9cfc70a9..1f379bf4 100644 --- a/proto/ospf/config.Y +++ b/proto/ospf/config.Y @@ -126,7 +126,7 @@ static inline void ospf_check_defcost(int cost) { if ((cost <= 0) || (cost >= LSINFINITY)) - cf_error("Default cost must be in range 1-%d", LSINFINITY-1); + cf_error("Default cost must be in range 1-%u", LSINFINITY-1); } static inline void @@ -185,10 +185,10 @@ ospf_proto_item: | RFC1583COMPAT bool { OSPF_CFG->rfc1583 = $2; } | STUB ROUTER bool { OSPF_CFG->stub_router = $3; } | ECMP bool { OSPF_CFG->ecmp = $2 ? OSPF_DEFAULT_ECMP_LIMIT : 0; } - | ECMP bool LIMIT expr { OSPF_CFG->ecmp = $2 ? $4 : 0; if ($4 < 0) cf_error("ECMP limit cannot be negative"); } + | ECMP bool LIMIT expr { OSPF_CFG->ecmp = $2 ? $4 : 0; } | MERGE EXTERNAL bool { OSPF_CFG->merge_external = $3; } - | TICK expr { OSPF_CFG->tick = $2; if($2<=0) cf_error("Tick must be greater than zero"); } - | INSTANCE ID expr { OSPF_CFG->instance_id = $3; if (($3<0) || ($3>255)) cf_error("Instance ID must be in range 0-255"); } + | TICK expr { OSPF_CFG->tick = $2; if($2 <= 0) cf_error("Tick must be greater than zero"); } + | INSTANCE ID expr { OSPF_CFG->instance_id = $3; if ($3 > 255) cf_error("Instance ID must be in range 0-255"); } | ospf_area ; @@ -318,7 +318,7 @@ ospf_iface_item: | REAL BROADCAST bool { OSPF_PATT->real_bcast = $3; if (!ospf_cfg_is_v2()) cf_error("Real broadcast option requires OSPFv2"); } | PTP NETMASK bool { OSPF_PATT->ptp_netmask = $3; if (!ospf_cfg_is_v2()) cf_error("PtP netmask option requires OSPFv2"); } | TRANSMIT DELAY expr { OSPF_PATT->inftransdelay = $3 ; if (($3<=0) || ($3>65535)) cf_error("Transmit delay must be in range 1-65535"); } - | PRIORITY expr { OSPF_PATT->priority = $2 ; if (($2<0) || ($2>255)) cf_error("Priority must be in range 0-255"); } + | PRIORITY expr { OSPF_PATT->priority = $2 ; if ($2>255) cf_error("Priority must be in range 0-255"); } | STRICT NONBROADCAST bool { OSPF_PATT->strictnbma = $3 ; } | STUB bool { OSPF_PATT->stub = $2 ; } | CHECK LINK bool { OSPF_PATT->check_link = $3; } @@ -404,7 +404,7 @@ ospf_iface_start: ospf_instance_id: /* empty */ - | INSTANCE expr { OSPF_PATT->instance_id = $2; if (($2<0) || ($2>255)) cf_error("Instance ID must be in range 0-255"); } + | INSTANCE expr { OSPF_PATT->instance_id = $2; if ($2 > 255) cf_error("Instance ID must be in range 0-255"); } ; ospf_iface_patt_list: diff --git a/proto/radv/config.Y b/proto/radv/config.Y index 7ba23205..b5f4b5f2 100644 --- a/proto/radv/config.Y +++ b/proto/radv/config.Y @@ -91,14 +91,14 @@ radv_iface_item: | MIN DELAY expr { RADV_IFACE->min_delay = $3; if ($3 <= 0) cf_error("Min delay must be positive"); } | MANAGED bool { RADV_IFACE->managed = $2; } | OTHER CONFIG bool { RADV_IFACE->other_config = $3; } - | LINK MTU expr { RADV_IFACE->link_mtu = $3; if ($3 < 0) cf_error("Link MTU must be 0 or positive"); } - | REACHABLE TIME expr { RADV_IFACE->reachable_time = $3; if (($3 < 0) || ($3 > 3600000)) cf_error("Reachable time must be in range 0-3600000"); } - | RETRANS TIMER expr { RADV_IFACE->retrans_timer = $3; if ($3 < 0) cf_error("Retrans timer must be 0 or positive"); } - | CURRENT HOP LIMIT expr { RADV_IFACE->current_hop_limit = $4; if (($4 < 0) || ($4 > 255)) cf_error("Current hop limit must be in range 0-255"); } + | LINK MTU expr { RADV_IFACE->link_mtu = $3; } + | REACHABLE TIME expr { RADV_IFACE->reachable_time = $3; if ($3 > 3600000) cf_error("Reachable time must be in range 0-3600000"); } + | RETRANS TIMER expr { RADV_IFACE->retrans_timer = $3; } + | CURRENT HOP LIMIT expr { RADV_IFACE->current_hop_limit = $4; if ($4 > 255) cf_error("Current hop limit must be in range 0-255"); } | DEFAULT LIFETIME expr radv_sensitive { RADV_IFACE->default_lifetime = $3; - if (($3 < 0) || ($3 > 9000)) cf_error("Default lifetime must be in range 0-9000"); - if ($4 != -1) RADV_IFACE->default_lifetime_sensitive = $4; + if ($3 > 9000) cf_error("Default lifetime must be in range 0-9000"); + if ($4 != (uint) -1) RADV_IFACE->default_lifetime_sensitive = $4; } | DEFAULT PREFERENCE radv_preference { RADV_IFACE->default_preference = $3; } | PREFIX radv_prefix { add_tail(&RADV_IFACE->pref_list, NODE this_radv_prefix); } @@ -125,7 +125,7 @@ radv_iface_finish: if ((ic->min_ra_int > 3) && (ic->min_ra_int > (ic->max_ra_int * 3 / 4))) - cf_error("Min RA interval must be at most 3/4 * Max RA interval %d %d", ic->min_ra_int, ic->max_ra_int); + cf_error("Min RA interval must be at most 3/4 * Max RA interval"); if ((ic->default_lifetime > 0) && (ic->default_lifetime < ic->max_ra_int)) cf_error("Default lifetime must be either 0 or at least Max RA interval"); @@ -163,13 +163,11 @@ radv_prefix_item: | AUTONOMOUS bool { RADV_PREFIX->autonomous = $2; } | VALID LIFETIME expr radv_sensitive { RADV_PREFIX->valid_lifetime = $3; - if ($3 < 0) cf_error("Valid lifetime must be 0 or positive"); - if ($4 != -1) RADV_PREFIX->valid_lifetime_sensitive = $4; + if ($4 != (uint) -1) RADV_PREFIX->valid_lifetime_sensitive = $4; } | PREFERRED LIFETIME expr radv_sensitive { RADV_PREFIX->preferred_lifetime = $3; - if ($3 < 0) cf_error("Preferred lifetime must be 0 or positive"); - if ($4 != -1) RADV_PREFIX->preferred_lifetime_sensitive = $4; + if ($4 != (uint) -1) RADV_PREFIX->preferred_lifetime_sensitive = $4; } ; diff --git a/proto/rip/config.Y b/proto/rip/config.Y index 25c2ac7a..46f6c177 100644 --- a/proto/rip/config.Y +++ b/proto/rip/config.Y @@ -65,7 +65,7 @@ rip_proto_item: proto_item | proto_channel | ECMP bool { RIP_CFG->ecmp = $2 ? RIP_DEFAULT_ECMP_LIMIT : 0; } - | ECMP bool LIMIT expr { RIP_CFG->ecmp = $2 ? $4 : 0; if ($4 < 0) cf_error("ECMP limit cannot be negative"); } + | ECMP bool LIMIT expr { RIP_CFG->ecmp = $2 ? $4 : 0; } | INFINITY expr { RIP_CFG->infinity = $2; } | INTERFACE rip_iface ; From c72b660b7423b0fb687794b722884cd6e5e6c562 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Tue, 23 May 2017 18:39:20 +0200 Subject: [PATCH 07/10] Client: Fix isspace() calls Function isspace() expects to get *unsigned* chars (encoded as ints), not that it matters for plain ASCII. --- client/commands.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/client/commands.c b/client/commands.c index 29e71418..0da7d835 100644 --- a/client/commands.c +++ b/client/commands.c @@ -36,6 +36,8 @@ struct cmd_node { static struct cmd_node cmd_root; +#define isspace_(X) isspace((unsigned char) (X)) + void cmd_build_tree(void) { @@ -53,7 +55,7 @@ cmd_build_tree(void) while (*c) { char *d = c; - while (*c && !isspace(*c)) + while (*c && !isspace_(*c)) c++; for(new=old->son; new; new=new->sibling) if (new->len == c-d && !memcmp(new->token, d, c-d)) @@ -71,7 +73,7 @@ cmd_build_tree(void) new->prio = (new->len == 3 && (!memcmp(new->token, "roa", 3) || !memcmp(new->token, "rip", 3))) ? 0 : 1; /* Hack */ } old = new; - while (isspace(*c)) + while (isspace_(*c)) c++; } if (cmd->is_real_cmd) @@ -132,7 +134,7 @@ cmd_list_ambiguous(struct cmd_node *root, char *cmd, int len) struct cmd_node *m; for(m=root->son; m; m=m->sibling) - if (m->len > len && !memcmp(m->token, cmd, len)) + if (m->len > len && !memcmp(m->token, cmd, len)) cmd_display_help(m->help, m->cmd); } @@ -147,13 +149,13 @@ cmd_help(char *cmd, int len) n = &cmd_root; while (cmd < end) { - if (isspace(*cmd)) + if (isspace_(*cmd)) { cmd++; continue; } z = cmd; - while (cmd < end && !isspace(*cmd)) + while (cmd < end && !isspace_(*cmd)) cmd++; m = cmd_find_abbrev(n, z, cmd-z, &ambig); if (ambig) @@ -222,20 +224,20 @@ cmd_complete(char *cmd, int len, char *buf, int again) int ambig, cnt = 0, common; /* Find the last word we want to complete */ - for(fin=end; fin > start && !isspace(fin[-1]); fin--) + for(fin=end; fin > start && !isspace_(fin[-1]); fin--) ; /* Find the context */ n = &cmd_root; while (cmd < fin && n->son) { - if (isspace(*cmd)) + if (isspace_(*cmd)) { cmd++; continue; } z = cmd; - while (cmd < fin && !isspace(*cmd)) + while (cmd < fin && !isspace_(*cmd)) cmd++; m = cmd_find_abbrev(n, z, cmd-z, &ambig); if (ambig) @@ -290,13 +292,13 @@ cmd_expand(char *cmd) n = &cmd_root; while (*c) { - if (isspace(*c)) + if (isspace_(*c)) { c++; continue; } b = c; - while (*c && !isspace(*c)) + while (*c && !isspace_(*c)) c++; m = cmd_find_abbrev(n, b, c-b, &ambig); if (!m) From b7761af34dc4ed3f1bdf874eb85d743b931b0af6 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Thu, 25 May 2017 23:30:39 +0200 Subject: [PATCH 08/10] Conf: Replace keyword and symbol hash table with generic hash table. The old hash table had fixed size, which makes it slow for config files with large number of symbols and symbol lookups. The new one is growing according to needs. --- conf/cf-lex.l | 168 ++++++++++++++++++++------------------------------ conf/conf.c | 4 +- conf/conf.h | 12 +++- nest/cmds.c | 26 ++++---- 4 files changed, 93 insertions(+), 117 deletions(-) diff --git a/conf/cf-lex.l b/conf/cf-lex.l index e9e385a6..620c119c 100644 --- a/conf/cf-lex.l +++ b/conf/cf-lex.l @@ -48,6 +48,7 @@ #include "conf/conf.h" #include "conf/cf-parse.tab.h" #include "lib/string.h" +#include "lib/hash.h" struct keyword { byte *name; @@ -57,22 +58,32 @@ struct keyword { #include "conf/keywords.h" -#define KW_HASH_SIZE 64 -static struct keyword *kw_hash[KW_HASH_SIZE]; -static int kw_hash_inited; -#define SYM_HASH_SIZE 128 +static uint cf_hash(byte *c); + +#define KW_KEY(n) n->name +#define KW_NEXT(n) n->next +#define KW_EQ(a,b) !strcmp(a,b) +#define KW_FN(k) cf_hash(k) +#define KW_ORDER 8 /* Fixed */ + +#define SYM_KEY(n) n->name, n->scope->active +#define SYM_NEXT(n) n->next +#define SYM_EQ(a,s1,b,s2) !strcmp(a,b) && s1 == s2 +#define SYM_FN(k,s) cf_hash(k) +#define SYM_ORDER 6 /* Initial */ + +#define SYM_REHASH sym_rehash +#define SYM_PARAMS /8, *1, 2, 2, 6, 20 + + +HASH_DEFINE_REHASH_FN(SYM, struct symbol) + +HASH(struct keyword) kw_hash; + -struct sym_scope { - struct sym_scope *next; /* Next on scope stack */ - struct symbol *name; /* Name of this scope */ - int active; /* Currently entered */ -}; static struct sym_scope *conf_this_scope; -static int cf_hash(byte *c); -static inline struct symbol * cf_get_sym(byte *c, uint h0); - linpool *cfg_mem; int (*cf_read_hook)(byte *buf, unsigned int max, int fd); @@ -179,23 +190,20 @@ else: { yytext[yyleng-1] = 0; yytext++; } - unsigned int h = cf_hash(yytext); - struct keyword *k = kw_hash[h & (KW_HASH_SIZE-1)]; - while (k) + + struct keyword *k = HASH_FIND(kw_hash, KW, yytext); + if (k) + { + if (k->value > 0) + return k->value; + else { - if (!strcmp(k->name, yytext)) - { - if (k->value > 0) - return k->value; - else - { - cf_lval.i = -k->value; - return ENUM; - } - } - k=k->next; + cf_lval.i = -k->value; + return ENUM; } - cf_lval.s = cf_get_sym(yytext, h); + } + + cf_lval.s = cf_get_symbol(yytext); return SYM; } @@ -257,13 +265,13 @@ else: { %% -static int +static uint cf_hash(byte *c) { - unsigned int h = 13; + uint h = 13 << 24; while (*c) - h = (h * 37) + *c++; + h = h + (h >> 2) + (h >> 5) + ((uint) *c++ << 24); return h; } @@ -428,58 +436,29 @@ check_eof(void) } static struct symbol * -cf_new_sym(byte *c, uint h0) +cf_new_symbol(byte *c) { - uint h = h0 & (SYM_HASH_SIZE-1); - struct symbol *s, **ht; - int l; + struct symbol *s; - if (!new_config->sym_hash) - new_config->sym_hash = cfg_allocz(SYM_HASH_SIZE * sizeof(struct keyword *)); - ht = new_config->sym_hash; - l = strlen(c); + uint l = strlen(c); if (l > SYM_MAX_LEN) cf_error("Symbol too long"); + s = cfg_alloc(sizeof(struct symbol) + l); - s->next = ht[h]; - ht[h] = s; s->scope = conf_this_scope; s->class = SYM_VOID; s->def = NULL; s->aux = 0; strcpy(s->name, c); + + if (!new_config->sym_hash.data) + HASH_INIT(new_config->sym_hash, new_config->pool, SYM_ORDER); + + HASH_INSERT2(new_config->sym_hash, SYM, new_config->pool, s); + return s; } -static struct symbol * -cf_find_sym(struct config *cfg, byte *c, uint h0) -{ - uint h = h0 & (SYM_HASH_SIZE-1); - struct symbol *s, **ht; - - if (ht = cfg->sym_hash) - { - for(s = ht[h]; s; s=s->next) - if (!strcmp(s->name, c) && s->scope->active) - return s; - } - if (ht = cfg->sym_fallback) - { - /* We know only top-level scope is active */ - for(s = ht[h]; s; s=s->next) - if (!strcmp(s->name, c) && s->scope->active) - return s; - } - - return NULL; -} - -static inline struct symbol * -cf_get_sym(byte *c, uint h0) -{ - return cf_find_sym(new_config, c, h0) ?: cf_new_sym(c, h0); -} - /** * cf_find_symbol - find a symbol by name * @cfg: specificed config @@ -494,7 +473,18 @@ cf_get_sym(byte *c, uint h0) struct symbol * cf_find_symbol(struct config *cfg, byte *c) { - return cf_find_sym(cfg, c, cf_hash(c)); + struct symbol *s; + + if (cfg->sym_hash.data && + (s = HASH_FIND(cfg->sym_hash, SYM, c, 1))) + return s; + + if (cfg->fallback && + cfg->fallback->sym_hash.data && + (s = HASH_FIND(cfg->fallback->sym_hash, SYM, c, 1))) + return s; + + return NULL; } /** @@ -509,7 +499,7 @@ cf_find_symbol(struct config *cfg, byte *c) struct symbol * cf_get_symbol(byte *c) { - return cf_get_sym(c, cf_hash(c)); + return cf_find_symbol(new_config, c) ?: cf_new_symbol(c); } struct symbol * @@ -522,7 +512,7 @@ cf_default_name(char *template, int *counter) for(;;) { bsprintf(buf, template, ++(*counter)); - s = cf_get_sym(buf, cf_hash(buf)); + s = cf_get_symbol(buf); if (s->class == SYM_VOID) return s; if (!perc) @@ -553,7 +543,7 @@ cf_define_symbol(struct symbol *sym, int type, void *def) { if (sym->scope == conf_this_scope) cf_error("Symbol already defined"); - sym = cf_new_sym(sym->name, cf_hash(sym->name)); + sym = cf_new_symbol(sym->name); } sym->class = type; sym->def = def; @@ -563,15 +553,11 @@ cf_define_symbol(struct symbol *sym, int type, void *def) static void cf_lex_init_kh(void) { - struct keyword *k; + HASH_INIT(kw_hash, &root_pool, KW_ORDER); - for(k=keyword_list; k->name; k++) - { - unsigned h = cf_hash(k->name) & (KW_HASH_SIZE-1); - k->next = kw_hash[h]; - kw_hash[h] = k; - } - kw_hash_inited = 1; + struct keyword *k; + for (k=keyword_list; k->name; k++) + HASH_INSERT(kw_hash, KW, k); } /** @@ -585,7 +571,7 @@ cf_lex_init_kh(void) void cf_lex_init(int is_cli, struct config *c) { - if (!kw_hash_inited) + if (!kw_hash.data) cf_lex_init_kh(); ifs_head = ifs = push_ifs(NULL); @@ -644,24 +630,6 @@ cf_pop_scope(void) ASSERT(conf_this_scope); } -struct symbol * -cf_walk_symbols(struct config *cf, struct symbol *sym, int *pos) -{ - for(;;) - { - if (!sym) - { - if (*pos >= SYM_HASH_SIZE) - return NULL; - sym = cf->sym_hash[(*pos)++]; - } - else - sym = sym->next; - if (sym && sym->scope->active) - return sym; - } -} - /** * cf_symbol_class_name - get name of a symbol class * @sym: symbol diff --git a/conf/conf.c b/conf/conf.c index 0a4e3f8c..7f4eb7e8 100644 --- a/conf/conf.c +++ b/conf/conf.c @@ -163,7 +163,7 @@ int cli_parse(struct config *c) { int done = 0; - c->sym_fallback = config->sym_hash; + c->fallback = config; new_config = c; cfg_mem = c->mem; if (setjmp(conf_jmpbuf)) @@ -174,7 +174,7 @@ cli_parse(struct config *c) done = 1; cleanup: - c->sym_fallback = NULL; + c->fallback = NULL; new_config = NULL; cfg_mem = NULL; return done; diff --git a/conf/conf.h b/conf/conf.h index 41cb434f..bf74b76b 100644 --- a/conf/conf.h +++ b/conf/conf.h @@ -11,6 +11,7 @@ #include "lib/resource.h" #include "lib/timer.h" +#include "lib/hash.h" /* Configuration structure */ @@ -50,8 +51,8 @@ struct config { char *err_file_name; /* File name containing error */ char *file_name; /* Name of main configuration file */ int file_fd; /* File descriptor of main configuration file */ - struct symbol **sym_hash; /* Lexer: symbol hash table */ - struct symbol **sym_fallback; /* Lexer: fallback symbol hash table */ + HASH(struct symbol) sym_hash; /* Lexer: symbol hash table */ + struct config *fallback; /* Link to regular config for CLI parsing */ int obstacle_count; /* Number of items blocking freeing of this config */ int shutdown; /* This is a pseudo-config for daemon shutdown */ bird_clock_t load_time; /* When we've got this configuration */ @@ -112,6 +113,12 @@ struct symbol { char name[1]; }; +struct sym_scope { + struct sym_scope *next; /* Next on scope stack */ + struct symbol *name; /* Name of this scope */ + int active; /* Currently entered */ +}; + #define SYM_MAX_LEN 64 /* Remember to update cf_symbol_class_name() */ @@ -154,7 +161,6 @@ struct symbol *cf_default_name(char *template, int *counter); struct symbol *cf_define_symbol(struct symbol *symbol, int type, void *def); void cf_push_scope(struct symbol *); void cf_pop_scope(void); -struct symbol *cf_walk_symbols(struct config *cf, struct symbol *sym, int *pos); char *cf_symbol_class_name(struct symbol *sym); static inline int cf_symbol_is_constant(struct symbol *sym) diff --git a/nest/cmds.c b/nest/cmds.c index 70fbdaf8..0bc9b9d1 100644 --- a/nest/cmds.c +++ b/nest/cmds.c @@ -46,22 +46,24 @@ cmd_show_status(void) void cmd_show_symbols(struct sym_show_data *sd) { - int pos = 0; - struct symbol *sym = sd->sym; - - if (sym) - cli_msg(1010, "%-8s\t%s", sym->name, cf_symbol_class_name(sym)); + if (sd->sym) + cli_msg(1010, "%-8s\t%s", sd->sym->name, cf_symbol_class_name(sd->sym)); else + { + HASH_WALK(config->sym_hash, next, sym) { - while (sym = cf_walk_symbols(config, sym, &pos)) - { - if (sd->type && (sym->class != sd->type)) - continue; + if (!sym->scope->active) + continue; - cli_msg(-1010, "%-8s\t%s", sym->name, cf_symbol_class_name(sym)); - } - cli_msg(0, ""); + if (sd->type && (sym->class != sd->type)) + continue; + + cli_msg(-1010, "%-8s\t%s", sym->name, cf_symbol_class_name(sym)); } + HASH_WALK_END; + + cli_msg(0, ""); + } } static void From 4fec43067e27c7a6c20a6ef9909bef0238984a64 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Tue, 30 May 2017 14:43:49 +0200 Subject: [PATCH 09/10] Workaround for older bisons --- conf/cf-lex.l | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/conf/cf-lex.l b/conf/cf-lex.l index 620c119c..66be3811 100644 --- a/conf/cf-lex.l +++ b/conf/cf-lex.l @@ -58,6 +58,11 @@ struct keyword { #include "conf/keywords.h" +/* Could be defined by Bison in cf-parse.tab.h, inteferes with SYM hash */ +#ifdef SYM +#undef SYM +#endif + static uint cf_hash(byte *c); From 33f7fbc42d0490b27e33275d0fc74d3ef55683e4 Mon Sep 17 00:00:00 2001 From: "Ondrej Zajicek (work)" Date: Wed, 31 May 2017 13:31:03 +0200 Subject: [PATCH 10/10] CLI: Fix bug in symbol handling introduced in previous patches --- nest/cli.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/nest/cli.c b/nest/cli.c index 83e79616..face409d 100644 --- a/nest/cli.c +++ b/nest/cli.c @@ -261,6 +261,7 @@ cli_command(struct cli *c) log(L_TRACE "CLI: %s", c->rx_buf); bzero(&f, sizeof(f)); f.mem = c->parser_pool; + f.pool = rp_new(c->pool, "Config"); cf_read_hook = cli_cmd_read_hook; cli_rh_pos = c->rx_buf; cli_rh_len = strlen(c->rx_buf); @@ -270,6 +271,8 @@ cli_command(struct cli *c) res = cli_parse(&f); if (!res) cli_printf(c, 9001, f.err_msg); + + config_free(&f); } static void