From: Alberto Leiva Popper Date: Fri, 21 Dec 2018 06:04:47 +0000 (-0600) Subject: Review of rsync.c code X-Git-Tag: v0.0.2~113 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e62240a0a1c60b67c911920fb000e702bcf2f6f2;p=thirdparty%2FFORT-validator.git Review of rsync.c code Still need to review the calling code and actually test it. --- diff --git a/src/log.h b/src/log.h index ac4dd963..365ec403 100644 --- a/src/log.h +++ b/src/log.h @@ -1,6 +1,15 @@ #ifndef SRC_LOG_H_ #define SRC_LOG_H_ +/* + * I know that the OpenBSD style guide says that we shouldn't declare our own + * error printing functions, but we kind of need to do it: + * + * - It's convoluted to use err() and warn() on libcrypto errors. + * - If debug is enabled, we want the error messages to be printed as a tree + * to ease debugging. + */ + void pr_debug(const char *, ...); void pr_debug_add(const char *, ...); void pr_debug_rm(const char *, ...); diff --git a/src/main.c b/src/main.c index e12ab697..c0457c1d 100644 --- a/src/main.c +++ b/src/main.c @@ -105,17 +105,15 @@ main(int argc, char **argv) if (argc < 3) return pr_err("Repository path as first argument and TAL file as second argument, please."); - if (argc >= 4) is_rsync_active = false; - error = rsync_init(is_rsync_active); + error = hash_init(); if (error) return error; - - error = hash_init(); + error = rsync_init(is_rsync_active); if (error) - return error; /* TODO revert rsync? */ + return error; add_rpki_oids(); thvar_init(); @@ -126,12 +124,11 @@ main(int argc, char **argv) repository_len = strlen(repository); error = tal_load(argv[2], &tal); - if (error) - return error; - - error = foreach_uri(tal, handle_tal_uri); + if (!error) { + error = foreach_uri(tal, handle_tal_uri); + tal_destroy(tal); + } - tal_destroy(tal); rsync_destroy(); return error; } diff --git a/src/rsync/rsync.c b/src/rsync/rsync.c index a9598bf0..992a39cf 100644 --- a/src/rsync/rsync.c +++ b/src/rsync/rsync.c @@ -9,7 +9,8 @@ #include #include -#include "../common.h" +#include "common.h" +#include "log.h" struct uri { char *string; @@ -35,25 +36,28 @@ int rsync_init(bool is_rsync_active) { // TODO remove the next 2 lines + /* + * TODO (rsync) No, don't. Disabling rsync will forever be a useful + * debugging feature. + */ if (!is_rsync_active) { execute_rsync = is_rsync_active; return 0; } rsync_uris = malloc(sizeof(struct uri_list)); - if (!rsync_uris) - return -ENOMEM; + if (rsync_uris == NULL) + return pr_enomem(); SLIST_INIT(rsync_uris); return 0; } void -rsync_destroy() +rsync_destroy(void) { struct uri *uri; - // TODO remove the next 2 lines if (!execute_rsync) return; @@ -73,12 +77,16 @@ do_rsync(char const *rsync_uri) int error; char *command; char *dest; + /* TODO (rsync) comment is invisible in narrow editors */ char const *rsync_command = "rsync --recursive --delete --times --contimeout=20 "; /* space char at end*/ + /* TODO (rsync) dest is leaking */ error = get_dest_path(rsync_uri, &dest); if (error) return error; + /* TODO (rsync) It seems that rsync_command does not need a `+ 1` */ + /* TODO (rsync) line exceeds 80 column limit */ command = malloc(strlen(rsync_command) + 1 + strlen(rsync_uri) + 1 + strlen(dest) + 1); if (command == NULL) return -ENOMEM; @@ -90,18 +98,41 @@ do_rsync(char const *rsync_uri) free(dest); - printf("(%s) command = %s \n", __func__, command); + pr_debug("(%s) command = %s", __func__, command); error = system(command); if (error) { - printf("result rsync %d\n", error); - perror("rsync"); + int error2 = errno; + /* + * The error message needs to be really generic because it seems + * that the Linux system() and the OpenBSD system() return + * different things. + */ + pr_err("rsync returned nonzero. result:%d errno:%d", + error, error2); + if (error2) + pr_errno(error2, "The error message for errno is"); } free(command); return error; } +/* + * If @rsync_uri is a certificate, ghostbuster or manifest file, this returns + * its local location's parent. + * If @rsync_uri is anything else (including other file types), returns + * @rsync_uri's local location. + * + * TODO (rsync) Why are you doing this? is rsync incapable of synchronizing + * individual files? + * TODO (rsync) Also: That's wrong anyway. certificates, ghostbusters and + * manifests are not the only files RPKI has to handle. There's nothing in the + * RFCs requiring that only known file types be present, or even that all files + * must have extensions. + * If you REALLY need to tell the difference between files and directories, + * use stat(2) instead. + */ static int get_dest_path(char const *rsync_uri, char **result) { @@ -109,6 +140,7 @@ get_dest_path(char const *rsync_uri, char **result) unsigned int result_size; int error; + /* TODO (rsync) local_uri is leaking */ error = uri_g2l(rsync_uri, strlen(rsync_uri), &local_uri); if (error) return error; @@ -120,14 +152,13 @@ get_dest_path(char const *rsync_uri, char **result) temp_str = strrchr(local_uri, '/'); if (temp_str == NULL) { - // TODO warning msg - return -EINVAL; + return pr_err("URI '%s' has no slash.", local_uri); } result_size = temp_str - local_uri + 1; /* add slash (+1) */ temp_str = malloc(result_size + 1); /* null char (+1) */ if (temp_str == NULL) { - return -ENOMEM; + return pr_enomem(); } temp_str[result_size] = '\0'; /*Set null char*/ strncpy(temp_str, local_uri, result_size); @@ -137,39 +168,11 @@ get_dest_path(char const *rsync_uri, char **result) return 0; } -//static int -//do_rsync(char *rsync_uri) -//{ -// int temp, result; -// -// char *temp_char; -// char *rsync_command[] = {"rsync", "--recursive", "--delete", "--times", NULL, NULL, NULL}; -// char *src = "rsync://rpki.afrinic.net/repository/AfriNIC.cer"; -// char *dest = "/home/dhf/Desktop/rpkitest/rpki.afrinic.net/repository/"; -// -// rsync_command[4] = src; -// rsync_command[5] = dest; -// temp = 0; -// while (temp < 7) { -// temp_char = rsync_command[temp]; -// printf("[%d] ", temp); -// if (temp_char == NULL) { -// printf("NULL\n"); -// } else { -// printf("%s\n", temp_char); -// } -// temp++; -// } -// printf("pre execv \n"); -//// result = execve(rsync_command[0], rsync_command, NULL); -// -// result = system("rsync --recursive --delete --times rsync://rpki.afrinic.net/repository/AfriNIC.cer /home/dhf/Desktop/rpkitest/rpki.afrinic.net/repository/"); -//// printf("result execv %d\n", result); -// printf("result rsync %d\n", result); -// perror("rsync"); -// return 0; -//} - +/* + * Returns whether new_uri's prefix is rsync_uri. + * TODO (rsync) why does this not care about nodes? It will return true if + * `rsync_uri = proto://a/b/c` and `new_uri = proto://a/b/cc`. + */ static bool rsync_uri_prefix_equals(struct uri *rsync_uri, char const *new_uri) { @@ -179,6 +182,10 @@ rsync_uri_prefix_equals(struct uri *rsync_uri, char const *new_uri) if (rsync_uri->len > uri_len) return false; + /* + * TODO (rsync) Don't use '!' for tests unless it's a boolean. + * (OpenBSD style) + */ return !strncasecmp(rsync_uri->string, new_uri, rsync_uri->len); } @@ -188,6 +195,7 @@ is_uri_in_list(char const *rsync_uri) struct uri *cursor; bool found; + /* TODO (rsync) this if doesn't seem to be doing anything */ if (SLIST_EMPTY(rsync_uris)) { return false; } @@ -208,20 +216,23 @@ add_uri_to_list(char const *rsync_uri_path) { struct uri *rsync_uri; size_t urilen; + rsync_uri = malloc(sizeof(struct uri)); - if (rsync_uri == NULL) { - warnx("Out of memory"); - return -ENOMEM; - } + if (rsync_uri == NULL) + return pr_enomem(); urilen = strlen(rsync_uri_path); rsync_uri->string = malloc(urilen + 1); if (!rsync_uri->string) { free(rsync_uri); - warnx("Out of memory"); - return -ENOMEM; + return pr_enomem(); } + /* + * TODO (rsync) caller frees rsync_uri_path right after calling this. + * Transfer ownership instead so you don't need the extra allocate, copy + * and free. + */ strcpy(rsync_uri->string, rsync_uri_path); rsync_uri->len = urilen; SLIST_INSERT_HEAD(rsync_uris, rsync_uri, next); @@ -229,6 +240,24 @@ add_uri_to_list(char const *rsync_uri_path) return 0; } +/* + * Returns rsync_uri, truncated to the second significant slash. + * I have no idea why. + * + * Examples: + * + * rsync_uri: rsync://aa/bb/cc/dd/ee/ff/gg/hh/ii + * result: rsync://aa/bb/ + * + * rsync_uri: rsync://aa/bb/ + * result: rsync://aa/bb/ + * + * rsync_uri: rsync://aa/ + * result: rsync://aa// + * + * rsync_uri: rsync://aa + * result: rsync://aa + */ static int short_uri(char const *rsync_uri, char **result) { @@ -241,11 +270,18 @@ short_uri(char const *rsync_uri, char **result) prefix_len = strlen(PREFIX); if (strncmp(PREFIX, rsync_uri, prefix_len) != 0) { + /* TODO (rsync) why is this commented out? */ // pr_err("Global URI %s does not begin with '%s'.", rsync_uri, // PREFIX); return -EINVAL; } + /* + * TODO (rsync) It took me a while to notice that this loop does not + * actually iterate. Why did you add it? It's misleading. If it's + * because you wanted to break instead of goto in the `tmp == NULL` if, + * then this should be a separate function instead. + */ do { tmp = rsync_uri + prefix_len; tmp = strchr(tmp, '/'); @@ -283,18 +319,21 @@ download_files(char const *rsync_uri) int error; char *rsync_uri_path, *localuri; - // TODO remove the next 2 lines if (!execute_rsync) return 0; if (is_uri_in_list(rsync_uri)){ - printf("(%s) ON LIST: %s\n", __func__, rsync_uri); + pr_debug("(%s) ON LIST: %s", __func__, rsync_uri); error = 0; goto end; } else { - printf("(%s) DOWNLOAD: %s\n",__func__, rsync_uri); + pr_debug("(%s) DOWNLOAD: %s", __func__, rsync_uri); } + /* + * TODO (rsync) I don't understand why you need to do this. + * Please comment. + */ error = short_uri(rsync_uri, &rsync_uri_path); if (error) return error; @@ -312,6 +351,12 @@ download_files(char const *rsync_uri) if (error) goto free_uri_path; + /* + * TODO (rsync) This doesn't look right to me. + * The one you queried was rsync_uri. The one you're adding is + * rsync_uri_path. It looks like is_uri_in_list() will only match when + * short_uri() doesn't do anything. + */ error = add_uri_to_list(rsync_uri_path); free_uri_path: @@ -321,7 +366,6 @@ end: } - static int create_dir_recursive(char *localuri) { @@ -337,11 +381,26 @@ create_dir_recursive(char *localuri) localuri_len = strlen(localuri); repository_len = strlen(repository); - temp_luri = localuri + repository_len ; + temp_luri = localuri + repository_len; strcpy(path, repository); offset = repository_len; + /* + * TODO (rsync) You might have gone a little overboard with this. + * Are you just trying to mkdir -p localuri? + * If so, wouldn't this be enough? + * + * for (i = 1; localuri[i] != '\0'; i++) { + * if (localuri[i] == '/') { + * localuri[i] = '\0'; + * create_dir(localuri); // handle error etc etc + * localuri[i] = '/'; + * } + * } + * + * We're not in Java; our strings are mutable. + */ slash = strchr(temp_luri, '/'); while (slash != NULL) { @@ -388,11 +447,11 @@ is_file(char const *path) { size_t path_len = strlen(path); - if (file_has_extension(path, path_len, "cer")) + if (file_has_extension(path, path_len, ".cer")) return true; - if (file_has_extension(path, path_len, "gbr")) + if (file_has_extension(path, path_len, ".gbr")) return true; - if (file_has_extension(path, path_len, "mft")) + if (file_has_extension(path, path_len, ".mft")) return true; return false; @@ -403,10 +462,30 @@ dir_exist(char *path) { int error; struct stat _stat; + /* + * TODO (rsync) I don't know if you're aware of this, but you can + * usually run `man 2 [core c function]` or `man 3 [core c function]` + * and get a lot of info. + * `man 2 stat`, for example, contains a lot of stuff you clearly need + * to read. + */ error = stat(path, &_stat); - if (error == -1) + if (error == -1) { + /* + * TODO (rsync) Never do this. stat() can return -1 for a large + * number of reasons, and only one of them is "file not found." + * Return the error code and send the boolean as an out + * parameter. + */ return false; /* a dir or file not exist*/ + } + /* + * TODO (rsync) Don't do this either. The function is called "dir_exist" + * but you're returning true even if the node happens to be a file, + * a socket, a named pipe, a door, etc etc. What's going to happen if + * each of these files interact with calling code? + */ return true; } @@ -415,22 +494,47 @@ create_dir(char *path) { struct stat _stat; int error; + /* + * TODO (rsync) Does this really need to be a variable? + * You only use it once. + */ mode_t mode = 0777; + /* + * TODO (rsync) Again. The function is called "create_dir" but you're + * returning success on regular file found. And it's really weird that + * only .cer, .gbr and .mft files count as directories according to this + * if. + * + * Alternatively: As implemented, this if is redundant because the + * return 0 on successful stat below already succeeds on files. Not that + * I agree with the stat below either. + */ if (is_file(path)) return 0; error = stat(path, &_stat); if (error != -1) { - /* a dir or file exist*/ + /* a dir or file exist*/ /* TODO (rsync) STOP IT. */ return 0; } if (errno != ENOENT) { + /* + * TODO (rsync) Error message is unfriendly because the user + * has no context on what "stat" is. + * Something like "stat() failed" would be a little better. + * Use pr_errno() for the automatic errno string. (See `man 3 + * perror`) + */ perror("stat"); + /* + * TODO (rsync) No reason to lose the error code. Return errno + * (or better yet: the result of pr_errno(errno)) instead of -1. + */ return -1; /* another error occurs*/ } error = mkdir(path, mode); - return error; + return error; /* TODO (rsync) No message on error */ }