From: pcarana Date: Thu, 21 Mar 2019 01:37:50 +0000 (-0600) Subject: Fix several TODOs of review X-Git-Tag: v0.0.2~52^2~18 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=d8890c9f1ed472d9496bac1de35f57b0cae29865;p=thirdparty%2FFORT-validator.git Fix several TODOs of review --- diff --git a/src/address.c b/src/address.c index 0499e409..63639d13 100644 --- a/src/address.c +++ b/src/address.c @@ -85,13 +85,13 @@ prefix4_decode(const char *str, struct ipv4_prefix *result) int error; if (str == NULL) { - err(-EINVAL, "Null string received, can't decode IPv4 prefix"); + warnx("Null string received, can't decode IPv4 prefix"); return -EINVAL; } error = str2addr4(str, &result->addr); if (error) { - err(error, "Invalid IPv4 prefix %s", str); + warnx("Invalid IPv4 prefix '%s'", str); return error; } @@ -114,13 +114,13 @@ prefix6_decode(const char *str, struct ipv6_prefix *result) * Use `warn()`/`warnx()` instead (and continue throwing cleanly). */ if (str == NULL) { - err(-EINVAL, "Null string received, can't decode IPv6 prefix"); + warnx("Null string received, can't decode IPv6 prefix"); return -EINVAL; } error = str2addr6(str, &result->addr); if (error) { - err(error, "Invalid IPv6 prefix %s", str); + warnx("Invalid IPv6 prefix '%s'", str); return error; } @@ -142,20 +142,20 @@ prefix_length_decode (const char *text, unsigned int *dst, int max_value) * 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"); + warnx("Null string received, can't decode prefix length"); return -EINVAL; } errno = 0; len = strtoul(text, NULL, 10); if (errno) { - err(errno, "Invalid prefix length '%s': %s", text, + warn("Invalid prefix length '%s': %s", text, strerror(errno)); return -EINVAL; } /* An underflow or overflow will be considered here */ if (len < 0 || max_value < len) { - err(-EINVAL, "Prefix length (%ld) is out of bounds (0-%d).", + warnx("Prefix length (%ld) is out of bounds (0-%d).", len, max_value); return -EINVAL; } @@ -169,7 +169,7 @@ prefix4_validate (struct ipv4_prefix *prefix) char buffer[INET_ADDRSTRLEN]; if ((prefix->addr.s_addr & be32_suffix_mask(prefix->len)) != 0) { - err(-EINVAL, "IPv4 prefix %s/%u has enabled suffix bits.", + warn("IPv4 prefix %s/%u has enabled suffix bits.", addr2str4(&prefix->addr, buffer), prefix->len); return -EINVAL; } @@ -188,7 +188,7 @@ prefix6_validate (struct ipv6_prefix *prefix) || (prefix->addr.s6_addr32[1] & suffix.s6_addr32[1]) || (prefix->addr.s6_addr32[2] & suffix.s6_addr32[2]) || (prefix->addr.s6_addr32[3] & suffix.s6_addr32[3])) { - err(-EINVAL, "IPv6 prefix %s/%u has enabled suffix bits.", + warn("IPv6 prefix %s/%u has enabled suffix bits.", addr2str6(&prefix->addr, buffer), prefix->len); return -EINVAL; } diff --git a/src/csv.c b/src/csv.c index 77ef191b..e3fe7c6a 100644 --- a/src/csv.c +++ b/src/csv.c @@ -13,22 +13,6 @@ #include "line_file.h" #include "vrps.h" -/* @ext must include the period. */ -static bool -location_has_extension(char const *location, char const *ext) -{ - size_t ext_len, loc_len; - int cmp; - - ext_len = strlen(ext); - loc_len = strlen(location); - if (loc_len < ext_len) - return false; - - cmp = strncmp(location + loc_len - ext_len, ext, ext_len); - return cmp == 0; -} - static int parse_asn(char *text, unsigned int *value) { @@ -36,7 +20,7 @@ parse_asn(char *text, unsigned int *value) char *start; if (text == NULL) { - err(-EINVAL, "Null string received, can't decode ASN"); + warnx("Null string received, can't decode ASN"); return -EINVAL; } @@ -47,12 +31,12 @@ parse_asn(char *text, unsigned int *value) errno = 0; asn = strtoul(start, NULL, 10); if (errno) { - err(errno, "Invalid ASN '%s': %s", text, strerror(errno)); + warn("Invalid ASN '%s'", text); return -EINVAL; } /* An underflow or overflow will be considered here */ if (asn < 0 || UINT32_MAX < asn) { - err(-EINVAL, "Prefix length (%lu) is out of bounds (0-%u).", + warnx("Prefix length (%lu) is out of bounds (0-%u).", asn, UINT32_MAX); return -EINVAL; } @@ -96,6 +80,11 @@ add_vrp(char *line, struct delta *delta) bool isv4; char *token, *line_copy; + if (strcmp(line, "") == 0) { + warnx("Empty line."); + return -EINVAL; + } + line_copy = malloc(strlen(line) + 1); if (line_copy == NULL) { error = -ENOMEM; @@ -199,33 +188,17 @@ 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); - if (error) { - err(error, "Error at first line, stop processing CSV file."); - return error; - } - if (line == NULL) { - error = -EINVAL; - err(error, "Empty CSV file, stop processing."); - return error; - } - /* Start the initial delta */ - /* TODO (review) check NULL */ + /* Init delta */ delta = create_delta(); + if (delta == NULL) { + warn("Couldn't allocate new delta"); + return -ENOMEM; + } + current_line = 1; do { - ++current_line; error = lfile_read(lfile, &line); if (error) { - err(error, "Error reading line %d, stop processing file.", + warn("Error reading line %d, stop processing file.", current_line); delta_destroy(&delta); goto end; @@ -234,24 +207,19 @@ load_vrps(struct line_file *lfile, bool is_update) error = 0; goto persist; } - if (strcmp(line, "") == 0) { - warn("There's nothing at line %d, ignoring.", - current_line); - continue; - } - /* TODO (review) line is leaking. */ error = add_vrp(line, delta); - if (error) { - delta_destroy(&delta); - goto end; - } + if (error) + warnx("Ignoring content at line %d.", current_line); + + current_line++; + free(line); } while (true); persist: /* TODO (review) delta is leaking. */ error = deltas_db_add_delta(delta); if (error) - err(error, "VRPs Delta couldn't be persisted"); + warnx("VRPs Delta couldn't be persisted"); end: if (line != NULL) free(line); @@ -270,58 +238,36 @@ load_vrps_file(bool check_update, bool *updated) 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. */ - /* Look for the last update date */ error = stat(location, &attr); if (error) { warn("Couldn't get last modified date of %s, skip update", location); - goto end2; + goto end; } 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; + if (check_update && + difftime(last_update, get_vrps_last_modified_date()) <= 0) + goto end; + + error = lfile_open(location, &lfile); + if (error) + goto end; /* Error msg already printed. */ error = load_vrps(lfile, check_update); if (error) - goto end2; + goto close_file; if (updated != NULL) *updated = check_update && - last_update > get_vrps_last_modified_date(); + difftime(last_update, get_vrps_last_modified_date()) > 0; set_vrps_last_modified_date(last_update); -end2: +close_file: lfile_close(lfile); -end1: +end: return error; }