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 9c1c0ebd0d1a7abf4455d70e844eeea25670ef4f..aac23fcc8767c0d5b5207b46a230c65720322ec6 100644 (file)
--- 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 352b468da56c95eebf9e6388eb3764809b7d4278..7eb285a38f80cf09019c92f4f9e7665c4379068b 100644 (file)
--- 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 ef1e01e714124f1fdd35c64a51d2fd3e7511b89e..b607c98f5f4f797c76f62c6044052face75ac671 100644 (file)
--- 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 ebb9ff8b1ab624a85f98a1f4710584953acfbd3a..8cf9d6e372b7886230e6c1eb3b7a818e6e1c9ae3 100644 (file)
--- 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 4eda91bffbb2f2af29734781884e59ab125df4f2..ffd4a5dc1d7a1ebbe6e57cb147830ce36bf69e5e 100644 (file)
--- 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();
        }