From: Alberto Leiva Popper Date: Mon, 13 May 2019 13:55:32 +0000 (-0500) Subject: Second iteration of the client responses review X-Git-Tag: v0.0.2~36 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=93ac47639d72e68e222c38d375cd84fb50aa91db;p=thirdparty%2FFORT-validator.git Second iteration of the client responses review --- diff --git a/src/clients.c b/src/clients.c index f5611f98..df5e6e1c 100644 --- a/src/clients.c +++ b/src/clients.c @@ -49,20 +49,7 @@ create_client(struct rtr_client *client, struct hashable_client **result) return pr_enomem(); node->meat.fd = client->fd; - node->meat.family = client->addr.ss_family; - switch (client->addr.ss_family) { - case AF_INET: - node->meat.sin = SADDR_IN(&client->addr)->sin_addr; - node->meat.sin_port = SADDR_IN(&client->addr)->sin_port; - break; - case AF_INET6: - node->meat.sin6 = SADDR_IN6(&client->addr)->sin6_addr; - node->meat.sin_port = SADDR_IN6(&client->addr)->sin6_port; - break; - default: - free(node); - return pr_crit("Bad protocol: %u", client->addr.ss_family); - } + node->meat.serial_number_set = false; *result = node; return 0; diff --git a/src/clients.h b/src/clients.h index b014e915..be95e575 100644 --- a/src/clients.h +++ b/src/clients.h @@ -1,19 +1,15 @@ #ifndef SRC_CLIENTS_H_ #define SRC_CLIENTS_H_ -#include +#include #include "rtr/pdu.h" #include "rtr/db/vrp.h" struct client { int fd; - sa_family_t family; - union { - struct in_addr sin; - struct in6_addr sin6; - }; - in_port_t sin_port; + serial_t serial_number; + bool serial_number_set; }; int clients_db_init(void); diff --git a/src/rtr/db/vrps.c b/src/rtr/db/vrps.c index c9a8aef4..b8292b95 100644 --- a/src/rtr/db/vrps.c +++ b/src/rtr/db/vrps.c @@ -264,7 +264,14 @@ revert_base: return error; } -/* TODO (whatever) @serial is a dumb hack. */ +/** + * Please keep in mind that there is at least one errcode-aware caller. The most + * important ones are + * 1. 0: No errors. + * 2. -EAGAIN: No data available; database still under construction. + * + * TODO (whatever) @serial is a dumb hack. + */ int vrps_foreach_base_roa(vrp_foreach_cb cb, void *arg, serial_t *serial) { @@ -289,12 +296,11 @@ vrps_foreach_base_roa(vrp_foreach_cb cb, void *arg, serial_t *serial) /** * Adds to @result the deltas whose serial > @from. * - * The result code is one of the following: - * - * 0: No errors. - * -EAGAIN: No data available; database still under construction. - * -ESRCH: @from was not found. - * Anything else: Generic fail. + * Please keep in mind that there is at least one errcode-aware caller. The most + * important ones are + * 1. 0: No errors. + * 2. -EAGAIN: No data available; database still under construction. + * 3. -ESRCH: @from was not found. * * As usual, only 0 guarantees valid out parameters. (@to and @result.) * (But note that @result is supposed to be already initialized, so caller will diff --git a/src/rtr/err_pdu.c b/src/rtr/err_pdu.c index c8cb7f4c..3be96ef0 100644 --- a/src/rtr/err_pdu.c +++ b/src/rtr/err_pdu.c @@ -4,36 +4,43 @@ #include "pdu_sender.h" #include "log.h" +typedef enum rtr_error_code { + ERR_PDU_CORRUPT_DATA = 0, + ERR_PDU_INTERNAL_ERROR = 1, + ERR_PDU_NO_DATA_AVAILABLE = 2, + ERR_PDU_INVALID_REQUEST = 3, + ERR_PDU_UNSUP_PROTO_VERSION = 4, + ERR_PDU_UNSUP_PDU_TYPE = 5, + ERR_PDU_WITHDRAWAL_UNKNOWN = 6, + ERR_PDU_DUPLICATE_ANNOUNCE = 7, + /* RTRv1 only, so not used yet. */ + ERR_PDU_UNEXPECTED_PROTO_VERSION = 8, +} rtr_error_code_t; + /* * TODO (urgent) According to the function below, NO_DATA_AVAILABLE is not * fatal. However, some callers of this function are terminating the connection * regardless of that. */ static int -err_pdu_send(int fd, uint16_t code, struct rtr_request const *request, +err_pdu_send(int fd, rtr_error_code_t code, struct rtr_request const *request, char const *message_const) { + char *message; + /* * This function must always return error so callers can interrupt * themselves easily. + * But note that not all callers should use this. + * TODO (now) Prevent errors to errors + * (It's harder than it seems, because request->pdu is sometimes NULL.) */ - int error; - char *message; - - /* TODO (now) Prevent errors to errors */ - message = (message_const != NULL) ? strdup(message_const) : NULL; - error = send_error_report_pdu(fd, code, request, message); + send_error_report_pdu(fd, code, request, message); free(message); - if (err_pdu_is_fatal(code)) { - pr_warn("Fatal error report PDU sent [code %u], closing socket.", - code); - close(fd); - } - - return error ? error : -EINVAL; + return -EINVAL; } int @@ -43,6 +50,11 @@ err_pdu_send_corrupt_data(int fd, struct rtr_request const *request, return err_pdu_send(fd, ERR_PDU_CORRUPT_DATA, request, message); } +/* + * Please note: If you're planning to send this error due to a memory + * allocation failure, you probably shouldn't; you'd likely only aggravate the + * problem. + */ int err_pdu_send_internal_error(int fd) { @@ -84,48 +96,39 @@ err_pdu_send_unsupported_pdu_type(int fd, struct rtr_request const *request) bool err_pdu_is_fatal(uint16_t code) { - /* Only NO_DATA_AVAILABLE error isn't fatal */ + /* + * Only NO_DATA_AVAILABLE error isn't fatal + * + * Addendum: Note that this is only non-fatal if we're the ones sending + * it. If the clients is the one telling us this, then it probably + * counts as "erroneous Error Report PDU", which is totally fatal. + */ return code != ERR_PDU_NO_DATA_AVAILABLE; } -void -err_pdu_log(uint16_t code, char *message) +char const * +err_pdu_to_string(uint16_t code) { - char const *code_title; - - switch (code) { + switch ((rtr_error_code_t) code) { case ERR_PDU_CORRUPT_DATA: - code_title = "Corrupt Data"; - break; + return "Corrupt Data"; case ERR_PDU_INTERNAL_ERROR: - code_title = "Internal Error"; - break; + return "Internal Error"; case ERR_PDU_NO_DATA_AVAILABLE: - code_title = "No Data Available"; - break; + return "No Data Available"; case ERR_PDU_INVALID_REQUEST: - code_title = "Invalid Request"; - break; + return "Invalid Request"; case ERR_PDU_UNSUP_PROTO_VERSION: - code_title = "Unsupported Protocol Version"; - break; + return "Unsupported Protocol Version"; case ERR_PDU_UNSUP_PDU_TYPE: - code_title = "Unsupported PDU Type"; - break; + return "Unsupported PDU Type"; case ERR_PDU_WITHDRAWAL_UNKNOWN: - code_title = "Withdrawal of Unknown Record"; - break; + return "Withdrawal of Unknown Record"; case ERR_PDU_DUPLICATE_ANNOUNCE: - code_title = "Duplicate Announcement Received"; - break; + return "Duplicate Announcement Received"; case ERR_PDU_UNEXPECTED_PROTO_VERSION: - code_title = "Unexpected Protocol Version"; - break; - default: - code_title = "Unknown error code"; - break; + return "Unexpected Protocol Version"; } - pr_err("Error report PDU info: '%s', message '%s'.", - code_title, message == NULL ? "[empty]" : message); + return "Unknown error code"; } diff --git a/src/rtr/err_pdu.h b/src/rtr/err_pdu.h index 0ac9ffd3..4082b6c6 100644 --- a/src/rtr/err_pdu.h +++ b/src/rtr/err_pdu.h @@ -6,16 +6,6 @@ #include "rtr/pdu.h" -#define ERR_PDU_CORRUPT_DATA 0 -#define ERR_PDU_INTERNAL_ERROR 1 -#define ERR_PDU_NO_DATA_AVAILABLE 2 -#define ERR_PDU_INVALID_REQUEST 3 -#define ERR_PDU_UNSUP_PROTO_VERSION 4 -#define ERR_PDU_UNSUP_PDU_TYPE 5 -#define ERR_PDU_WITHDRAWAL_UNKNOWN 6 -#define ERR_PDU_DUPLICATE_ANNOUNCE 7 -#define ERR_PDU_UNEXPECTED_PROTO_VERSION 8 - /* * Wrappers for err_pdu_send(). * Mainly, this is for the sake of making it easier to see whether the error is @@ -29,6 +19,6 @@ int err_pdu_send_invalid_request_truncated(int, unsigned char *, char const *); int err_pdu_send_unsupported_pdu_type(int, struct rtr_request const *); bool err_pdu_is_fatal(uint16_t); -void err_pdu_log(uint16_t, char *); +char const *err_pdu_to_string(uint16_t); #endif /* SRC_RTR_ERR_PDU_H_ */ diff --git a/src/rtr/pdu.c b/src/rtr/pdu.c index 9070af00..67878018 100644 --- a/src/rtr/pdu.c +++ b/src/rtr/pdu.c @@ -29,7 +29,10 @@ pdu_load(int fd, struct rtr_request *request, int error; /* Read the header into its buffer. */ - /* TODO If the first read yields no bytes, the connection was terminated. */ + /* + * TODO (urgent) If the first read yields no bytes, the connection was + * terminated. We're not ending gracefully in those cases. + */ error = pdu_reader_init(&reader, fd, hdr_bytes, RTRPDU_HEADER_LEN); if (error) /* Communication interrupted; omit error response */ @@ -53,6 +56,7 @@ pdu_load(int fd, struct rtr_request *request, * Error messages can be quite large. * But they're probably not legitimate, so drop 'em. * 512 is like a 5-paragraph error message, so it's probably enough. + * (Warning: I'm assuming english tho.) */ if (header.length > 512) { pr_warn("Got an extremely large PDU (%u bytes). WTF?", @@ -65,6 +69,7 @@ pdu_load(int fd, struct rtr_request *request, request->bytes_len = header.length; request->bytes = malloc(header.length); if (request->bytes == NULL) + /* No error report PDU on allocation failures. */ return pr_enomem(); memcpy(request->bytes, hdr_bytes, RTRPDU_HEADER_LEN); @@ -72,6 +77,7 @@ pdu_load(int fd, struct rtr_request *request, request->bytes + RTRPDU_HEADER_LEN, header.length - RTRPDU_HEADER_LEN); if (error) + /* Communication interrupted; no error PDU. */ goto revert_bytes; /* Deserialize the PDU. */ @@ -88,8 +94,10 @@ pdu_load(int fd, struct rtr_request *request, } error = meta->from_stream(&header, &reader, request->pdu); - if (error) + if (error) { + err_pdu_send_internal_error(fd); goto revert_pdu; + } /* Happy path. */ *metadata = meta; diff --git a/src/rtr/pdu.h b/src/rtr/pdu.h index dd809f22..2f982183 100644 --- a/src/rtr/pdu.h +++ b/src/rtr/pdu.h @@ -11,6 +11,10 @@ struct rtr_client { int fd; + /* + * TODO (whatever) this is currently not being used for anything. + * (But consider the end_client() to-do.) + */ struct sockaddr_storage addr; }; @@ -138,7 +142,14 @@ struct error_report_pdu { struct pdu_metadata { size_t length; + /** + * Caller assumes that from_stream functions are only allowed to fail + * on programming errors. (Because of the internal error response.) + */ int (*from_stream)(struct pdu_header *, struct pdu_reader *, void *); + /** + * Handlers are expected to send error PDUs on discretion. + */ int (*handle)(int, struct rtr_request const *); void (*destructor)(void *); }; diff --git a/src/rtr/pdu_handler.c b/src/rtr/pdu_handler.c index f48af5dc..798e7bd1 100644 --- a/src/rtr/pdu_handler.c +++ b/src/rtr/pdu_handler.c @@ -10,14 +10,9 @@ #include "pdu_sender.h" #include "rtr/db/vrps.h" -static int -warn_unexpected_pdu(int fd, struct rtr_request const *request, - char const *pdu_name) -{ - err_pdu_send_invalid_request(fd, request, - "PDU is unexpected or out of order."); - return -EINVAL; -} +#define warn_unexpected_pdu(fd, request, pdu_name) \ + err_pdu_send_invalid_request(fd, request, \ + "Clients are not supposed to send " pdu_name " PDUs."); int handle_serial_notify_pdu(int fd, struct rtr_request const *request) @@ -56,27 +51,42 @@ handle_serial_query_pdu(int fd, struct rtr_request const *request) deltas_db_init(&deltas); error = vrps_get_deltas_from(query->serial_number, &final_serial, &deltas); - if (error == -EAGAIN) { + switch (error) { + case 0: + break; + case -EAGAIN: /* Database still under construction */ err_pdu_send_no_data_available(fd); - error = 0; + error = 0; /* Don't panic; client should retry later */ goto end; - } - if (error == -ESRCH) { + case -ESRCH: /* Invalid serial */ /* https://tools.ietf.org/html/rfc6810#section-6.3 */ error = send_cache_reset_pdu(fd); goto end; - } - if (error) + case -ENOMEM: /* Memory allocation failure */ + goto end; + case EAGAIN: /* Too many threads */ + /* + * I think this should be more of a "try again" thing, but + * RTR does not provide a code for that. Just fall through. + */ + default: + error = err_pdu_send_internal_error(fd); goto end; + } - /* https://tools.ietf.org/html/rfc6810#section-6.2 */ + /* + * https://tools.ietf.org/html/rfc6810#section-6.2 + * + * These functions presently only fail on writes, allocations and + * programming errors. Best avoid error PDUs. + */ error = send_cache_response_pdu(fd); if (error) goto end; error = send_delta_pdus(fd, &deltas); if (error) - goto end; /* TODO (now) maybe send something? */ + goto end; error = send_end_of_data_pdu(fd, final_serial); end: @@ -102,7 +112,6 @@ send_base_roa(struct vrp const *vrp, void *arg) args->started = true; } - /* TODO (now) maybe send something on error? */ return send_prefix_pdu(args->fd, vrp, FLAG_ANNOUNCEMENT); } @@ -119,17 +128,23 @@ handle_reset_query_pdu(int fd, struct rtr_request const *request) /* * It's probably best not to work on a copy, because the tree is large. * Unfortunately, this means we'll have to encourage writer stagnation, - * but most clients are supposed to request far more serial queries than - * reset queries. + * but thankfully, most clients are supposed to request far more serial + * queries than reset queries. */ error = vrps_foreach_base_roa(send_base_roa, &args, ¤t_serial); - if (error == -EAGAIN) { + + /* See handle_serial_query_pdu() for some comments. */ + switch (error) { + case 0: + break; + case -EAGAIN: err_pdu_send_no_data_available(fd); return 0; - } - if (error) + case EAGAIN: + err_pdu_send_internal_error(fd); return error; + } return send_end_of_data_pdu(fd, current_serial); } @@ -174,13 +189,16 @@ int handle_error_report_pdu(int fd, struct rtr_request const *request) { struct error_report_pdu *received = request->pdu; + char const *error_name; - if (err_pdu_is_fatal(received->header.m.error_code)) { - pr_warn("Fatal error report PDU received [code %u], closing socket.", - received->header.m.error_code); - close(fd); - } - err_pdu_log(received->header.m.error_code, received->error_message); + error_name = err_pdu_to_string(received->header.m.error_code); - return 0; + if (received->error_message != NULL) + pr_info("Client responded with error PDU '%s' ('%s'). Closing socket.", + error_name, received->error_message); + else + pr_info("Client responded with error PDU '%s'. Closing socket.", + error_name); + + return -EINVAL; } diff --git a/src/rtr/pdu_sender.c b/src/rtr/pdu_sender.c index 4625b898..98eedfff 100644 --- a/src/rtr/pdu_sender.c +++ b/src/rtr/pdu_sender.c @@ -283,7 +283,7 @@ send_delta_pdus(int fd, struct deltas_db *deltas) struct vrp_slist filtered_vrps; struct delta_group *group; struct vrp_node *ptr; - int error; + int error = 0; /* * Short circuit: Entries that share serial are already guaranteed to diff --git a/src/rtr/rtr.c b/src/rtr/rtr.c index 3a7630d6..cadc2a04 100644 --- a/src/rtr/rtr.c +++ b/src/rtr/rtr.c @@ -158,6 +158,13 @@ clean_request(struct rtr_request *request, const struct pdu_metadata *meta) static void * end_client(struct rtr_client *client) { + /* + * TODO It'd probably be a good idea to print the client's address in + * this message. + */ + if (close(client->fd) != 0) + pr_errno(errno, "close() failed on client socket"); + clients_forget(client->fd); return NULL; } @@ -176,25 +183,15 @@ client_thread_cb(void *arg) while (loop) { /* For each PDU... */ error = pdu_load(client->fd, &request, &meta); if (error) - return end_client(client); - - error = clients_add(client); - if (error) { - clean_request(&request, meta); - err_pdu_send_internal_error(client->fd); - return end_client(client); - } + break; error = meta->handle(client->fd, &request); - if (error) { - clean_request(&request, meta); - return end_client(client); - } - clean_request(&request, meta); + if (error) + break; } - return NULL; /* Unreachable. */ + return end_client(client); } /* @@ -207,6 +204,7 @@ handle_client_connections(int server_fd) struct thread_node *new_thread; socklen_t sizeof_client_addr; int client_fd; + int error; listen(server_fd, config_get_server_queue()); @@ -231,8 +229,16 @@ handle_client_connections(int server_fd) * So don't interrupt the thread when this happens. */ + /* + * TODO this is more complicated than it needs to be. + * We have a client hash table and a thread linked list. + * These two contain essentially the same entries. It's + * redundant. + */ + new_thread = malloc(sizeof(struct thread_node)); if (new_thread == NULL) { + /* No error response PDU on memory allocation. */ pr_err("Couldn't create thread struct"); close(client_fd); continue; @@ -241,10 +247,24 @@ handle_client_connections(int server_fd) new_thread->client.fd = client_fd; new_thread->client.addr = client_addr; - errno = pthread_create(&new_thread->tid, NULL, + error = clients_add(&new_thread->client); + if (error) { + /* + * Presently, clients_add() can only fail due to alloc + * failure. No error report PDU. + */ + free(new_thread); + close(client_fd); + continue; + } + + error = pthread_create(&new_thread->tid, NULL, client_thread_cb, &new_thread->client); - if (errno) { - pr_errno(errno, "Could not spawn the client's thread"); + if (error != EAGAIN) + err_pdu_send_internal_error(client_fd); + if (error) { + pr_errno(error, "Could not spawn the client's thread"); + clients_forget(client_fd); free(new_thread); close(client_fd); continue;