From 29b76cae47b15b1f0fc663fc53d323a6e9aaf801 Mon Sep 17 00:00:00 2001 From: Aurelien DARRAGON Date: Thu, 19 Oct 2023 10:59:26 +0200 Subject: [PATCH] BUG/MEDIUM: server/log: "mode log" after server keyword causes crash In 9a74a6c ("MAJOR: log: introduce log backends"), a mistake was made: it was assumed that the proxy mode was already known during server keyword parsing in parse_server() function, but this is wrong. Indeed, "mode log" can be declared late in the proxy section. Due to this, a simple config like this will cause the process to crash: |backend test | | server name 127.0.0.1:8080 | mode log In order to fix this, we relax some checks in _srv_parse_init() and store the address protocol from str2sa_range() in server struct, then we set-up a postparsing function that is to be called after config parsing to finish the server checks/initialization that depend on the proxy mode to be known. We achieve this by checking the PR_CAP_LB capability from the parent proxy to know if we're in such case where the effective proxy mode is not yet known (it is assumed that other proxies which are implicit ones don't provide this possibility and thus don't suffer from this constraint). Only then, if the capability is not found, we immediately perform the server checks that depend on the proxy mode, else the check is postponed and it will automatically be performed during postparsing thanks to the REGISTER_POST_SERVER_CHECK() hook. Note that we remove the SRV_PARSE_IN_LOG_BE flag because it was introduced in the above commit and it is no longer relevant. No backport needed unless 9a74a6c gets backported. --- include/haproxy/server-t.h | 2 +- src/cfgparse-listen.c | 3 - src/server.c | 131 +++++++++++++++++++++++++++---------- 3 files changed, 99 insertions(+), 37 deletions(-) diff --git a/include/haproxy/server-t.h b/include/haproxy/server-t.h index 9b03600034..6d0fa0fba9 100644 --- a/include/haproxy/server-t.h +++ b/include/haproxy/server-t.h @@ -275,6 +275,7 @@ struct server { struct proxy *proxy; /* the proxy this server belongs to */ const struct mux_proto_list *mux_proto; /* the mux to use for all outgoing connections (specified by the "proto" keyword) */ + struct protocol *addr_proto; /* server side protocol used for haproxy<->server communication */ struct log_target *log_target; /* when 'mode log' is enabled, target facility used to transport log messages */ unsigned maxconn, minconn; /* max # of active sessions (0 = unlimited), min# for dynamic limit. */ struct srv_per_thread *per_thr; /* array of per-thread stuff such as connections lists */ @@ -626,7 +627,6 @@ struct srv_kw_list { #define SRV_PARSE_PARSE_ADDR 0x08 /* required to parse the server address in the second argument */ #define SRV_PARSE_DYNAMIC 0x10 /* dynamic server created at runtime with cli */ #define SRV_PARSE_INITIAL_RESOLVE 0x20 /* resolve immediately the fqdn to an ip address */ -#define SRV_PARSE_IN_LOG_BE 0x40 /* keyword in log backend */ #endif /* _HAPROXY_SERVER_T_H */ diff --git a/src/cfgparse-listen.c b/src/cfgparse-listen.c index 7ed67aa328..0b6f7fa075 100644 --- a/src/cfgparse-listen.c +++ b/src/cfgparse-listen.c @@ -438,7 +438,6 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) if ((strcmp(args[0], "server") == 0)) { err_code |= parse_server(file, linenum, args, curproxy, curr_defproxy, - (curproxy->mode == PR_MODE_SYSLOG ? SRV_PARSE_IN_LOG_BE : 0) | SRV_PARSE_PARSE_ADDR); if (err_code & ERR_FATAL) @@ -447,7 +446,6 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) else if (strcmp(args[0], "default-server") == 0) { err_code |= parse_server(file, linenum, args, curproxy, curr_defproxy, - (curproxy->mode == PR_MODE_SYSLOG ? SRV_PARSE_IN_LOG_BE : 0) | SRV_PARSE_DEFAULT_SERVER); if (err_code & ERR_FATAL) @@ -456,7 +454,6 @@ int cfg_parse_listen(const char *file, int linenum, char **args, int kwm) else if (strcmp(args[0], "server-template") == 0) { err_code |= parse_server(file, linenum, args, curproxy, curr_defproxy, - (curproxy->mode == PR_MODE_SYSLOG ? SRV_PARSE_IN_LOG_BE : 0) | SRV_PARSE_TEMPLATE|SRV_PARSE_PARSE_ADDR); if (err_code & ERR_FATAL) diff --git a/src/server.c b/src/server.c index 4873c1438a..ea6a325be6 100644 --- a/src/server.c +++ b/src/server.c @@ -2336,6 +2336,7 @@ void srv_settings_cpy(struct server *srv, const struct server *src, int srv_tmpl if (srv_tmpl) { srv->addr = src->addr; + srv->addr_proto = src->addr_proto; srv->svc_port = src->svc_port; } @@ -2706,6 +2707,93 @@ static int _srv_parse_tmpl_init(struct server *srv, struct proxy *px) return i - srv->tmpl_info.nb_low; } +/* Ensure server config will work with effective proxy mode + * + * This function is expected to be called after _srv_parse_init() initialization + * but only when the effective server's proxy mode is known, which is not always + * the case during parsing time, in which case the function will be called during + * postparsing thanks to the _srv_postparse() below. + * + * Returns ERR_NONE on success else a combination or ERR_CODE. + */ +static int _srv_check_proxy_mode(struct server *srv, char postparse) +{ + int err_code = ERR_NONE; + + if (postparse && !(srv->proxy->cap & PR_CAP_LB)) + return ERR_NONE; /* nothing to do, the check was already performed during parsing */ + + if (srv->conf.file) + set_usermsgs_ctx(srv->conf.file, srv->conf.line, NULL); + + if (!srv->addr_proto) { + /* proto may not be set (ie: if srv_parse_init() was skipped or + * SRV_PARSE_PARSE_ADDR was not set, in this case, we have nothing + * to check + */ + goto out; + } + if (srv->proxy->mode == PR_MODE_SYSLOG) { + /* mode log enabled: + * pre-resolve related log target from known infos + */ + + BUG_ON(srv->log_target); + srv->log_target = malloc(sizeof(*srv->log_target)); + if (!srv->log_target) { + ha_alert("memory error when allocating log server\n"); + err_code |= ERR_ALERT | ERR_FATAL; + goto out; + } + srv->log_target->addr = &srv->addr; + switch (srv->addr_proto->xprt_type) { + case PROTO_TYPE_DGRAM: + srv->log_target->type = LOG_TARGET_DGRAM; + break; + case PROTO_TYPE_STREAM: + /* for now BUFFER type only supports TCP server to it's almost + * explicit. This will require ring buffer creation during log + * postresolving step. + */ + srv->log_target->type = LOG_TARGET_BUFFER; + break; + default: + ha_alert("log server type not supported for log backend server.\n"); + err_code |= ERR_ALERT | ERR_FATAL; + break; + } + } + else { + /* for now, only TCP expected as srv's proto for other uses */ + if (srv->addr_proto->xprt_type != PROTO_TYPE_STREAM || !srv->addr_proto->connect) { + ha_alert("unsupported protocol for server address in '%s' backend.\n", proxy_mode_str(srv->proxy->mode)); + err_code |= ERR_ALERT | ERR_FATAL; + } + } + out: + if (srv->conf.file) + reset_usermsgs_ctx(); + + return err_code; +} + +/* Perform some server postparsing checks / tasks: + * We must be careful that checks / postinits performed within this function + * don't depend or conflict with other postcheck functions that are registered + * using REGISTER_POST_SERVER_CHECK() hook. + * + * Returns ERR_NONE on success else a combination or ERR_CODE. + */ +static int _srv_postparse(struct server *srv) +{ + int err_code = ERR_NONE; + + err_code |= _srv_check_proxy_mode(srv, 1); + + return err_code; +} +REGISTER_POST_SERVER_CHECK(_srv_postparse); + /* Allocate a new server pointed by and try to parse the first arguments * in as an address for a server or an address-range for a template or * nothing for a default-server. is incremented to the next argument. @@ -2725,7 +2813,6 @@ static int _srv_parse_init(struct server **srv, char **args, int *cur_arg, const char *err = NULL; int err_code = 0; char *fqdn = NULL; - struct protocol *proto; int tmpl_range_low = 0, tmpl_range_high = 0; char *errmsg = NULL; @@ -2815,12 +2902,11 @@ static int _srv_parse_init(struct server **srv, char **args, int *cur_arg, if (!(parse_flags & SRV_PARSE_PARSE_ADDR)) goto skip_addr; - sk = str2sa_range(args[*cur_arg], &port, &port1, &port2, NULL, &proto, + sk = str2sa_range(args[*cur_arg], &port, &port1, &port2, NULL, &newsrv->addr_proto, &errmsg, NULL, &fqdn, - (parse_flags & SRV_PARSE_IN_LOG_BE ? PA_O_DGRAM : PA_O_CONNECT) | (parse_flags & SRV_PARSE_INITIAL_RESOLVE ? PA_O_RESOLVE : 0) | PA_O_PORT_OK | (parse_flags & SRV_PARSE_IN_PEER_SECTION ? PA_O_PORT_MAND : PA_O_PORT_OFS) | - PA_O_STREAM | PA_O_XPRT); + PA_O_STREAM | PA_O_DGRAM | PA_O_XPRT); if (!sk) { ha_alert("%s\n", errmsg); err_code |= ERR_ALERT | ERR_FATAL; @@ -2858,35 +2944,6 @@ static int _srv_parse_init(struct server **srv, char **args, int *cur_arg, } } - if ((parse_flags & SRV_PARSE_IN_LOG_BE) && proto) { - /* mode log enabled, and found proto: - * pre-resolve related log target from known infos - */ - newsrv->log_target = malloc(sizeof(*newsrv->log_target)); - if (!newsrv->log_target) { - ha_alert("memory error when allocating log server\n"); - err_code |= ERR_ALERT | ERR_FATAL; - goto out; - } - newsrv->log_target->addr = &newsrv->addr; - switch (proto->xprt_type) { - case PROTO_TYPE_DGRAM: - newsrv->log_target->type = LOG_TARGET_DGRAM; - break; - case PROTO_TYPE_STREAM: - /* for now BUFFER type only supports TCP server to it's almost - * explicit. This will require ring buffer creation during log - * postresolving step. - */ - newsrv->log_target->type = LOG_TARGET_BUFFER; - break; - default: - ha_alert("log server type not supported for log backend server.\n"); - err_code |= ERR_ALERT | ERR_FATAL; - break; - } - } - newsrv->addr = *sk; newsrv->svc_port = port; /* @@ -2939,6 +2996,14 @@ static int _srv_parse_init(struct server **srv, char **args, int *cur_arg, } free(fqdn); + if (!(curproxy->cap & PR_CAP_LB)) { + /* No need to wait for effective proxy mode, it is already known: + * Only general purpose user-declared proxies ("listen", "frontend", "backend") + * offer the possibility to configure the mode of the proxy. Hopefully for us, + * they have the PR_CAP_LB set. + */ + return _srv_check_proxy_mode(newsrv, 0); + } return 0; out: -- 2.39.5