From: Art Cancro Date: Mon, 5 Feb 2024 22:46:34 +0000 (-0500) Subject: Awesome sauce, read description below for details. X-Git-Tag: v998~2 X-Git-Url: https://code.citadel.org/?p=citadel.git;a=commitdiff_plain;h=9f197d0b63b09d6cc295051016a5b549a65e8588 Awesome sauce, read description below for details. My 'ctdlload' operation kept failing because there were a few user records dumped that had insane data in them. CtdlDecodeBase64() was silently doing buffer overruns because it doesn't know the size of the target buffer. While noodling a workaround, I realized that you can pass the same pointer to CtdlDecodeBase64() as both the source *and* target buffer. This works because the decoded data pointer will ALWAYS be behind the encoded data pointer. This allows a "decode-in-place" operation! By performing a decode-in-place and then using safestrncpy() to copy the results to the finitely-sized string buffer in the target struct, we guarantee no buffer overruns. Valgrind no longer bitches about it and the program does not crash. --- diff --git a/citadel/utils/ctdldump.c b/citadel/utils/ctdldump.c index 9c1c0ebd0..aac23fcc8 100644 --- a/citadel/utils/ctdldump.c +++ b/citadel/utils/ctdldump.c @@ -1,6 +1,6 @@ // Dump the Citadel database to a flat file that can be restored by ctdlload on any architecture // -// Copyright (c) 2024 by Art Cancro citadel.org +// Copyright (c) 2023-2024 by Art Cancro citadel.org // // This program is open source software. Use, duplication, or disclosure // is subject to the terms of the GNU General Public License, version 3. diff --git a/citadel/utils/ctdlload.c b/citadel/utils/ctdlload.c index 352b468da..7eb285a38 100644 --- a/citadel/utils/ctdlload.c +++ b/citadel/utils/ctdlload.c @@ -1,6 +1,6 @@ // Load (restore) the Citadel database from a flat file created by ctdldump // -// Copyright (c) 2024 by Art Cancro citadel.org +// Copyright (c) 2023-2024 by Art Cancro citadel.org // // This program is open source software. Use, duplication, or disclosure // is subject to the terms of the GNU General Public License, version 3. @@ -71,8 +71,13 @@ int import_msgtext(char *line, struct cdbkeyval *kv) { // Convert a "msgmeta" record to a message metadata record on disk. NOT THREADSAFE int import_msgmeta(char *line, struct cdbkeyval *kv) { char *token; - struct MetaData *m = malloc(sizeof(struct MetaData)); + struct MetaData *m; + m = malloc(sizeof(struct MetaData)); + if (!m) { + fprintf(stderr, "import_msgmeta: malloc: %s\n", strerror(errno)); + return(0); + } memset(m, 0, sizeof(struct MetaData)); char *p = line; @@ -112,8 +117,13 @@ int import_msgmeta(char *line, struct cdbkeyval *kv) { int import_user(char *line, struct cdbkeyval *kv) { char userkey[USERNAME_SIZE]; char *token; - struct ctdluser *u = malloc(sizeof(struct ctdluser)); + struct ctdluser *u; + u = malloc(sizeof(struct ctdluser)); + if (!u) { + fprintf(stderr, "malloc failed\n"); + return(0); + } memset(u, 0, sizeof(struct ctdluser)); char *p = line; @@ -126,7 +136,7 @@ int import_user(char *line, struct cdbkeyval *kv) { u->uid = atoi(token); break; case 3: - strncpy(u->password, token, sizeof(u->password)); + safestrncpy(u->password, token, sizeof(u->password)); break; case 4: u->flags = atoi(token); @@ -144,7 +154,7 @@ int import_user(char *line, struct cdbkeyval *kv) { u->USuserpurge = atoi(token); break; case 9: - strncpy(u->fullname, token, sizeof(u->fullname)); + safestrncpy(u->fullname, token, sizeof(u->fullname)); break; case 10: u->msgnum_bio = atol(token); @@ -153,7 +163,8 @@ int import_user(char *line, struct cdbkeyval *kv) { u->msgnum_pic = atol(token); break; case 12: - CtdlDecodeBase64(u->emailaddrs, token, strlen(token)); + CtdlDecodeBase64(token, token, strlen(token)); + safestrncpy(u->emailaddrs, token, sizeof(u->emailaddrs)); break; case 13: u->msgnum_inboxrules = atol(token); @@ -163,7 +174,14 @@ int import_user(char *line, struct cdbkeyval *kv) { break; } } + + // reject any user records without names + if (strlen(u->fullname) == 0) { + free(u); + return(0); + } + // ok, good to go makeuserkey(userkey, u->fullname); kv->key.len = strlen(userkey); kv->key.ptr = strdup(userkey); @@ -176,8 +194,13 @@ int import_user(char *line, struct cdbkeyval *kv) { // Convert a "room" record to a record on disk. (Source string is unusable after this function is called.) int import_room(char *line, struct cdbkeyval *kv) { char *token; - struct ctdlroom *r = malloc(sizeof(struct ctdlroom)); + struct ctdlroom *r; + r = malloc(sizeof(struct ctdlroom)); + if (!r) { + fprintf(stderr, "import_room: malloc: %s\n", strerror(errno)); + return(0); + } memset(r, 0, sizeof(struct ctdlroom)); char *p = line; @@ -254,9 +277,14 @@ int import_room(char *line, struct cdbkeyval *kv) { // Convert a floor record to a record on disk. int import_floor(char *line, struct cdbkeyval *kv) { char *token; - struct floor *f = malloc(sizeof(struct floor)); + struct floor *f; int floor_num; + f = malloc(sizeof(struct floor)); + if (!f) { + fprintf(stderr, "import_floor: malloc: %s\n", strerror(errno)); + return(0); + } memset(f, 0, sizeof(struct floor)); char *p = line; @@ -332,8 +360,13 @@ int import_msglist(char *line, struct cdbkeyval *kv) { // Convert a "visit" record to a record on disk. int import_visit(char *line, struct cdbkeyval *kv) { char *token; - struct visit *v = malloc(sizeof(struct visit)); + struct visit *v; + v = malloc(sizeof(struct visit)); + if (!v) { + fprintf(stderr, "import_visit: malloc: %s\n", strerror(errno)); + return(0); + } memset(v, 0, sizeof(struct visit)); char *p = line; @@ -409,8 +442,13 @@ int import_dir(char *line, struct cdbkeyval *kv) { // Convert a "usetable" record to a record on disk. int import_usetable(char *line, struct cdbkeyval *kv) { char *token; - struct UseTable *u = malloc(sizeof(struct UseTable)); + struct UseTable *u; + u = malloc(sizeof(struct UseTable)); + if (!u) { + fprintf(stderr, "import_usetable: malloc: %s\n", strerror(errno)); + return(0); + } memset(u, 0, sizeof(struct UseTable)); char *p = line; @@ -579,7 +617,6 @@ void ingest_one(char *line, struct cdbkeyval *kv) { static int current_cdb = -1 ; char record_type[32]; int row_was_good; - static time_t last_diag = 0; // Clear out our record buffer memset(kv, 0, sizeof(struct cdbkeyval)); @@ -656,11 +693,9 @@ void ingest_one(char *line, struct cdbkeyval *kv) { else { ++bad_rows; } - if (time(NULL) > last_diag) { - fprintf(stderr, " \033[32m%9d\033[0m / \033[31m%8d\033[0m\r", good_rows, bad_rows); - fflush(stderr); - last_diag = time(NULL); - } + + fprintf(stderr, "\r \033[32m%9d\033[0m / \033[31m%8d\033[0m ", good_rows, bad_rows); + fflush(stderr); } @@ -726,13 +761,13 @@ int main(int argc, char **argv) { char *ctdldir = CTDLDIR; // display the greeting - fprintf(stderr, "\033[44m\033[1m" - "╔════════════════════════════════════════════════════════════════════════╗\n" - "║ DB Load utility for Citadel ║\n" - "║ Copyright (c) 2023-2024 by citadel.org et al. ║\n" - "║ This program is open source software. Use, duplication, or disclosure ║\n" - "║ is subject to the terms of the GNU General Public license v3. ║\n" - "╚════════════════════════════════════════════════════════════════════════╝\033[0m\n" + fprintf(stderr, + "\033[44m\033[1m╔════════════════════════════════════════════════════════════════════════╗\033[0m\n" + "\033[44m\033[1m║ DB Load utility for Citadel ║\033[0m\n" + "\033[44m\033[1m║ Copyright (c) 2023-2024 by citadel.org et al. ║\033[0m\n" + "\033[44m\033[1m║ This program is open source software. Use, duplication, or disclosure ║\033[0m\n" + "\033[44m\033[1m║ is subject to the terms of the GNU General Public license v3. ║\033[0m\n" + "\033[44m\033[1m╚════════════════════════════════════════════════════════════════════════╝\033[0m\n" ); // Parse command line @@ -777,6 +812,7 @@ int main(int argc, char **argv) { cdb_init_backends(); cdb_open_databases(); + // Load rows from the input source ingest(); // close databases diff --git a/libcitadel/lib/base64.c b/libcitadel/lib/base64.c index ef1e01e71..b607c98f5 100644 --- a/libcitadel/lib/base64.c +++ b/libcitadel/lib/base64.c @@ -5,7 +5,7 @@ // and I don't want to fix it again. I don't care how many nanoseconds you think // you can shave off the execution time. Don't fucking touch it. // -// Copyright (c) 1987-2022 by the citadel.org team +// Copyright (c) 1987-2024 by the citadel.org team // // This program is open source software. Use, duplication, or disclosure // is subject to the terms of the GNU General Public License, version 3. @@ -121,6 +121,11 @@ char b64unalphabet(char ch) { // source Source base64-encoded buffer // source_len Stop after parsing this many bytes // return value Decoded length + +// AWESOME SAUCE ALERT: +// It is legal to specify the same pointer for the source and destination buffers. +// If you do so, the string will be "decoded in place". + size_t CtdlDecodeBase64(char *dest, const char *source, size_t source_len) { size_t bytes_read = 0; size_t bytes_decoded = 0; diff --git a/libcitadel/lib/libcitadel.h b/libcitadel/lib/libcitadel.h index ebb9ff8b1..8cf9d6e37 100644 --- a/libcitadel/lib/libcitadel.h +++ b/libcitadel/lib/libcitadel.h @@ -402,7 +402,7 @@ enum { BASE64_NO_LINEBREAKS = 0, BASE64_YES_LINEBREAKS = 1 }; -size_t CtdlDecodeBase64(char *dest, const char *source, size_t length); +size_t CtdlDecodeBase64(char *dest, const char *source, size_t source_len); unsigned int decode_hex(char *Source); int CtdlDecodeQuotedPrintable(char *decoded, char *encoded, int sourcelen); void StripSlashes(char *Dir, int TrailingSlash); diff --git a/libcitadel/lib/tools.c b/libcitadel/lib/tools.c index 4eda91bff..ffd4a5dc1 100644 --- a/libcitadel/lib/tools.c +++ b/libcitadel/lib/tools.c @@ -34,8 +34,7 @@ typedef unsigned char byte; // Byte type int safestrncpy(char *dest, const char *src, size_t n) { int i = 0; - if (dest == NULL || src == NULL) - { + if (dest == NULL || src == NULL) { fprintf(stderr, "safestrncpy: NULL argument\n"); abort(); }