From: Alberto Leiva Popper Date: Wed, 20 Mar 2019 02:06:11 +0000 (-0600) Subject: Review, part one X-Git-Tag: v0.0.2~52^2~23 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=ce8448acbe66c370f8f435e30f4e38c7545c071b;p=thirdparty%2FFORT-validator.git Review, part one --- diff --git a/src/Makefile.am b/src/Makefile.am index ce5019eb..c3f85c26 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -7,7 +7,7 @@ rtr_server_SOURCES = main.c rtr_server_SOURCES += address.c address.h rtr_server_SOURCES += array_list.h rtr_server_SOURCES += clients.c clients.h -rtr_server_SOURCES += common.c common.h +rtr_server_SOURCES += common.h rtr_server_SOURCES += configuration.c configuration.h rtr_server_SOURCES += csv.c csv.h rtr_server_SOURCES += line_file.c line_file.h diff --git a/src/address.c b/src/address.c index f1b2a9c3..661f3420 100644 --- a/src/address.c +++ b/src/address.c @@ -103,6 +103,16 @@ prefix6_decode(const char *str, struct ipv6_prefix *result) { int error; + /* + * TODO (review) I see this pattern often: You call `err()`, and then + * throw a pretty exception. + * + * `err()` does not return, so any cleanup that follows it is pointless. + * + * More importantly, returnless functions are bad practice, + * because they destroy the recovery potential of calling code. + * Use `warn()`/`warnx()` instead (and continue throwing cleanly). + */ if (str == NULL) { err(-EINVAL, "Null string received, can't decode IPv6 prefix"); return -EINVAL; @@ -122,6 +132,15 @@ prefix_length_decode (const char *text, unsigned int *dst, int max_value) { unsigned long len; + /* + * TODO (review) You probably meant to use `errx()`, not `err()`. + * + * `err()` depends on a thread variable called `errno`. It makes no + * sense to call `err` when you know `errno` has not been set properly + * by some system function. + * + * If you haven't done it, see `man 3 err` and `man 3 errno`. + */ if (text == NULL) { err(-EINVAL, "Null string received, can't decode prefix length"); return -EINVAL; diff --git a/src/clients.c b/src/clients.c index ec95a9d6..e09bb4e5 100644 --- a/src/clients.c +++ b/src/clients.c @@ -7,6 +7,7 @@ ARRAY_LIST(clientsdb, struct client) +/* TODO (review) Does not need to live in the heap */ struct clientsdb *clients_db; int @@ -57,6 +58,7 @@ create_client(int fd, struct sockaddr_storage *addr, u_int8_t rtr_version) { struct client *client; + /* TODO (review) `client` should be in the stack. */ client = malloc(sizeof(struct client)); if (client == NULL) { err(-ENOMEM, "Couldn't allocate client"); @@ -95,6 +97,16 @@ update_client(int fd, struct sockaddr_storage *addr, u_int8_t rtr_version) if (client == NULL) return create_client(fd, addr, rtr_version); + /* + * TODO (review) Your margin seems to be misconfigured; you're violating + * the line width often. + * + * Line width is 80 characters and tabs are 8. + * + * `Window > Preferences > C/C++ > Code Style > Formatter > New... > Ok` + * `Indentation > Tab size` + * `Line Wrapping > Maximum line width` + */ /* * Isn't ready to handle distinct version on clients reconnection, but for * now there's no problem since only 1 RTR version is supported (RVR v0). diff --git a/src/common.c b/src/common.c deleted file mode 100644 index 1e748471..00000000 --- a/src/common.c +++ /dev/null @@ -1,14 +0,0 @@ -#include "common.h" - -#include -#include - -char * -str_clone(char const *original) -{ - char *result; - result = malloc(strlen(original) + 1); - if (result != NULL) - strcpy(result, original); - return result; -} diff --git a/src/common.h b/src/common.h index 7863baaf..e5fa6cf3 100644 --- a/src/common.h +++ b/src/common.h @@ -24,6 +24,4 @@ * does not. */ -char *str_clone(char const *); - #endif /* _SRC_COMMON_H_ */ diff --git a/src/configuration.c b/src/configuration.c index 18396059..d6ac72ec 100644 --- a/src/configuration.c +++ b/src/configuration.c @@ -174,7 +174,8 @@ handle_json(json_t *root) DEFAULT_VRPS_LOCATION, &vrps_location); if (error) return error; - config.vrps_location = str_clone(vrps_location); + /* TODO (review) check NULL */ + config.vrps_location = strdup(vrps_location); /* * RFC 6810 and 8210: @@ -295,7 +296,8 @@ init_addrinfo(char const *hostname, char const *service) return error; } - config.port = str_clone(service); + /* TODO (review) check NULL */ + config.port = strdup(service); return 0; } diff --git a/src/csv.c b/src/csv.c index d9323a92..98f9c997 100644 --- a/src/csv.c +++ b/src/csv.c @@ -165,6 +165,15 @@ add_vrp(char *line, struct delta *delta) goto error; } + /* + * TODO (review) You seem to be leaking `vrp` on success. (The arraylist + * stores a shallow copy, not the memory block you allocated.) + * + * In fact, `vrp_destroy()` is empty, so you're also leaking on error. + * + * You can avoid all these headaches by moving `vrp` to the stack. + * (ie. remove `vrp`'s asterisk and patch emerging errors.) + */ error = delta_add_vrp(delta, vrp); if (error) { vrp_destroy(vrp); @@ -186,6 +195,13 @@ load_vrps(struct line_file *lfile, bool is_update) int current_line; int error; + /* + * TODO (review) Skipping the first line should not be hardcoded; if + * there's no header, you will ignore important information. + * + * Try checking whether you need to handle the line by way of the "AS" + * prefix instead. + */ /* First line is expected to be the header, ignore it */ current_line = 1; error = lfile_read(lfile, &line); @@ -199,6 +215,7 @@ load_vrps(struct line_file *lfile, bool is_update) return error; } /* Start the initial delta */ + /* TODO (review) check NULL */ delta = create_delta(); do { ++current_line; @@ -217,6 +234,7 @@ load_vrps(struct line_file *lfile, bool is_update) continue; } + /* TODO (review) line is leaking. */ error = add_vrp(line, delta); if (error) { delta_destroy(&delta); @@ -224,6 +242,7 @@ load_vrps(struct line_file *lfile, bool is_update) } } while (true); persist: + /* TODO (review) delta is leaking. */ error = deltas_db_add_delta(delta); if (error) err(error, "VRPs Delta couldn't be persisted"); @@ -244,12 +263,23 @@ load_vrps_file(bool check_update, bool *updated) int error; location = config_get_vrps_location(); + + /* + * TODO (review) This is a needless bother; remove please. + * + * (Having a csv extension does not guarantee the file is CSV and + * vice versa.) + */ if (!location_has_extension(location, ".csv")) { warn("%s isn't a CSV file", location); error = -EINVAL; goto end1; } + /* + * TODO (review) This is an extremely heavy operation, and should not + * happen before you've confirmed that you need to read the file. + */ error = lfile_open(location, &lfile); if (error) goto end1; /* Error msg already printed. */ @@ -263,6 +293,13 @@ load_vrps_file(bool check_update, bool *updated) } last_update = attr.st_mtim.tv_sec; + /* + * TODO (review) Time comparisons are particularly vulnerable to integer + * overflow. (Think about `time_t` being defined as a small integer, + * and `get_vrps_last_modified_date()` is the max value.) + * + * Usa `difftime()` instead. (`man 3 difftime`) + */ if (check_update && last_update <= get_vrps_last_modified_date()) goto end2; @@ -282,6 +319,7 @@ end1: return error; } +/* TODO (review) Should be `csv_parse_vrps_file(void)`. */ int csv_parse_vrps_file() { diff --git a/src/main.c b/src/main.c index 3c327f2b..9022e695 100644 --- a/src/main.c +++ b/src/main.c @@ -36,6 +36,7 @@ main(int argc, char *argv[]) } } + /* TODO (review) I don't understand this comment; please ellaborate. */ /* TODO This will be overriden when reading from config file */ if (json_file == NULL) { fprintf(stderr, "Missing flag '-f '\n"); diff --git a/src/rtr/pdu_sender.c b/src/rtr/pdu_sender.c index 2c1c4137..9797053e 100644 --- a/src/rtr/pdu_sender.c +++ b/src/rtr/pdu_sender.c @@ -208,6 +208,10 @@ send_payload_pdus(struct sender_common *common) goto end; for (i = 0; i < len; i++) { + /* + * TODO (review) Why are you indexing `vrps`? You did not + * allocate an array. + */ if (vrps[i].in_addr_len == INET_ADDRSTRLEN) error = send_ipv4_prefix_pdu(common, &vrps[i]); else if (vrps[i].in_addr_len == INET6_ADDRSTRLEN) diff --git a/src/rtr/rtr.c b/src/rtr/rtr.c index ff07c22c..1822f995 100644 --- a/src/rtr/rtr.c +++ b/src/rtr/rtr.c @@ -168,6 +168,7 @@ handle_client_connections(int server_fd) struct thread_param *arg; pthread_t thread; + /* TODO (review) 5 should be configurable. */ listen(server_fd, 5); sizeof_client_addr = sizeof(client_addr); diff --git a/src/vrps.c b/src/vrps.c index 9d914aeb..0fcd08d2 100644 --- a/src/vrps.c +++ b/src/vrps.c @@ -1,5 +1,7 @@ #include "vrps.h" +/* TODO (review) Can't find the meaning of "VRP" and "VRPS". Please explain. */ + #include #include #include "array_list.h" @@ -15,10 +17,19 @@ struct delta { struct vrps vrps; }; +/* TODO (review) why pointers? */ ARRAY_LIST(deltasdb, struct delta *) +/* + * TODO (review) It seems you only have one instance of this. + * + * Best remove those asterisks; you don't need `base_db` and `deltas_db` to live + * in the heap. + */ struct state { + /** The current valid ROAs, freshly loaded from the file */ struct delta *base_db; + /** ROA changes over time */ struct deltasdb *deltas_db; u_int32_t current_serial; u_int16_t v0_session_id; @@ -48,8 +59,29 @@ deltas_db_init(void) err(error, "Deltas DB couldn't be initialized"); return error; } + + /* + * TODO (review) This looks dangerous. + * + * 1. RTR server starts with serial 0 + * 2. Router wants serial + * 3. RTR Server provides serial 0 + * 4. RTR Server dies + * 5. RTR Server starts with serial 0, but the repository has changed + * by this point + * + * Can they realize that they are out of sync? + * + * Can't you use the current date as serial? + */ state.current_serial = START_SERIAL; /* The downcast takes the LSBs */ + /* + * TODO (review) The result of `time()` is unlikely to fit in a 16-bit + * integer. + * + * (Also: Integer overflow yields undefined behavior.) + */ state.v0_session_id = time(NULL); /* Minus 1 to prevent same ID */ state.v1_session_id = state.v0_session_id - 1; @@ -157,6 +189,12 @@ vrp_is_new(struct vrps *base, struct vrp *vrp) return vrp_locate(base, vrp) == NULL; } +/* + * TODO (review) I don't understand the name of this function. + * + * I think that you meant "summarize." I've never seen "resume" used + * an spanish "resumen." + */ static struct delta * delta_resume(struct delta *delta) { @@ -164,6 +202,13 @@ delta_resume(struct delta *delta) struct vrps *base, *search_list; struct vrp *cursor; + /* + * Note: Don't fix this function yet. + * I realize why you implemented it this way, and I'm trying to come up + * with a more efficient algorithm. + */ + + /* TODO (review) check NULL */ resume_delta = create_delta(); resume_delta->serial = delta->serial; /* First check for announcements */ @@ -172,6 +217,7 @@ delta_resume(struct delta *delta) ARRAYLIST_FOREACH(base, cursor) if (vrp_is_new(search_list, cursor)) { cursor->flags = FLAG_ANNOUNCEMENT; + /* TODO (review) check error code */ delta_add_vrp(resume_delta, cursor); } @@ -181,6 +227,7 @@ delta_resume(struct delta *delta) ARRAYLIST_FOREACH(base, cursor) if (vrp_is_new(search_list, cursor)) { cursor->flags = FLAG_WITHDRAWAL; + /* TODO (review) check error code */ delta_add_vrp(resume_delta, cursor); } @@ -259,6 +306,16 @@ deltas_db_status(u_int32_t *serial) if (state.base_db->vrps.len == 0) return NO_DATA_AVAILABLE; + /* + * TODO (review) `//`-style comments are somewhat controversial in that + * they weren't added until some late standard. From my reading of it, + * the OpenBSD style does not approve of them either. + * + * If you're using Eclipse, go to `Window > Preferences > C/C++ > Code + * Analysis`, and set `Coding Style > Line Comments` to `Error`. + * + * Then patch all the emerging red underlines. + */ // No serial to match, and there's data at DB if (serial == NULL) return DIFF_AVAILABLE;