diff --git a/clientserver.c b/clientserver.c index 0a1f941d..e7bf9d39 100644 --- a/clientserver.c +++ b/clientserver.c @@ -275,8 +275,10 @@ int start_inband_exchange(int f_in, int f_out, const char *user, int argc, char if (!user) user = getenv("LOGNAME"); - if (exchange_protocols(f_in, f_out, line, sizeof line, 1) < 0) + if (exchange_protocols(f_in, f_out, line, sizeof line, 1) < 0) { + free(modname); return -1; + } if (early_input_file) { STRUCT_STAT st; @@ -289,12 +291,14 @@ int start_inband_exchange(int f_in, int f_out, const char *user, int argc, char rsyserr(FERROR, errno, "failed to open %s", early_input_file); if (f) fclose(f); + free(modname); return -1; } early_input_len = st.st_size; if (early_input_len > (int)sizeof line) { rprintf(FERROR, "%s is > %d bytes.\n", early_input_file, (int)sizeof line); fclose(f); + free(modname); return -1; } if (early_input_len > 0) { @@ -304,6 +308,7 @@ int start_inband_exchange(int f_in, int f_out, const char *user, int argc, char if (feof(f)) { rprintf(FERROR, "Early EOF in %s\n", early_input_file); fclose(f); + free(modname); return -1; } len = fread(line, 1, early_input_len, f); @@ -380,6 +385,7 @@ int start_inband_exchange(int f_in, int f_out, const char *user, int argc, char while (1) { if (!read_line_old(f_in, line, sizeof line, 0)) { rprintf(FERROR, "rsync: didn't get server startup line\n"); + free(modname); return -1; } @@ -403,6 +409,7 @@ int start_inband_exchange(int f_in, int f_out, const char *user, int argc, char rprintf(FERROR, "%s\n", line); /* This is always fatal; the server will now * close the socket. */ + free(modname); return -1; } diff --git a/flist.c b/flist.c index 968a20d1..64e694f1 100644 --- a/flist.c +++ b/flist.c @@ -1794,6 +1794,7 @@ static struct file_struct *send_file_name(int f, struct file_list *flist, sx.st.st_mode = file->mode; if (get_acl(fname, &sx) < 0) { io_error |= IOERR_GENERAL; + free_acl(&sx); return NULL; } } @@ -1803,6 +1804,9 @@ static struct file_struct *send_file_name(int f, struct file_list *flist, sx.st.st_mode = file->mode; if (get_xattr(fname, &sx) < 0) { io_error |= IOERR_GENERAL; +#ifdef SUPPORT_ACLS + free_acl(&sx); /* get_acl() above may have loaded one */ +#endif return NULL; } } diff --git a/generator.c b/generator.c index e562c449..e83d15f7 100644 --- a/generator.c +++ b/generator.c @@ -1441,7 +1441,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, static int need_fuzzy_dirlist = 0; struct file_struct *fuzzy_file = NULL; int fd = -1, f_copy = -1; - stat_x sx = {0}, real_sx; + stat_x sx = {0}, real_sx = {0}; STRUCT_STAT partial_st; struct file_struct *back_file = NULL; int statret, real_ret, stat_errno; @@ -2250,6 +2250,7 @@ static void recv_generator(char *fname, struct file_struct *file, int ndx, } free_stat_x(&sx); + free_stat_x(&real_sx); } /* If we are replacing an existing hard link, symlink, device, or special file, diff --git a/testsuite/inband-modname-leak_test.py b/testsuite/inband-modname-leak_test.py new file mode 100644 index 00000000..a8b41d96 --- /dev/null +++ b/testsuite/inband-modname-leak_test.py @@ -0,0 +1,67 @@ +#!/usr/bin/env python3 +"""KI-17: start_inband_exchange leaks modname on early error returns. + +clientserver.c:start_inband_exchange allocates `modname` (new_array) and frees +it only on the success path; the early `return -1` error paths (e.g. the +daemon replying @ERROR for an unknown module) leak it. Client-side, one-shot +per failed connection. + +ASan/LSan reproducer: connect to a daemon requesting a non-existent module so +the @ERROR path is taken, then assert no client leak report names +start_inband_exchange. Gated on an AddressSanitizer build and --use-tcp. +""" + +import glob +import os +import subprocess + +from rsyncfns import ( + FROMDIR, SCRATCHDIR, RSYNC, + make_tree, require_asan, require_tcp, rmtree, rsync_argv, + start_test_daemon, test_fail, +) + +DAEMON_PORT = 12896 +require_tcp("the daemon @ERROR handshake needs a real TCP peer") +require_asan("KI-17 modname leak is only observable under AddressSanitizer/LSan", RSYNC) + +src = FROMDIR +rmtree(src) +make_tree(src, depth=1) + +conf = SCRATCHDIR / 'modname-leak.conf' +conf.write_text( + f"pid file = {SCRATCHDIR}/rsyncd.pid\n" + "use chroot = no\n" + f"log file = {SCRATCHDIR}/rsyncd.log\n" + f"\n[realmod]\n\tpath = {src}\n\tread only = yes\n" +) +url = start_test_daemon(conf, DAEMON_PORT) + +asan_log = SCRATCHDIR / 'modname-leak-asan' +for stale in glob.glob(f"{asan_log}.*"): + os.unlink(stale) +os.environ['ASAN_OPTIONS'] = ( + f"detect_leaks=1:abort_on_error=0:log_path={asan_log}" +) + +# Request a module that does not exist: the daemon replies @ERROR and the +# client's start_inband_exchange takes the early `return -1` that leaks modname. +p = subprocess.run(rsync_argv('-r', f'{url}no-such-module/'), + capture_output=True, text=True) + +# Non-vacuity: the connection must have been refused via the @ERROR path that +# leaks modname (rc != 0 and an "Unknown module" / @ERROR diagnostic). +if p.returncode == 0: + test_fail("connection to a non-existent module unexpectedly succeeded; " + "the modname-leak path was not exercised") +if 'ERROR' not in p.stderr and 'nknown module' not in p.stderr: + test_fail(f"expected an @ERROR/unknown-module rejection; got:\n{p.stderr}") + +reports = ''.join(open(r, errors='replace').read() + for r in glob.glob(f"{asan_log}.*")) +if 'start_inband_exchange' in reports: + test_fail("start_inband_exchange leaked modname on the @ERROR path (KI-17):\n" + + reports[:1500]) + +print("inband-modname-leak: start_inband_exchange does not leak modname") diff --git a/testsuite/recv-generator-acl-leak_test.py b/testsuite/recv-generator-acl-leak_test.py new file mode 100644 index 00000000..402ed77d --- /dev/null +++ b/testsuite/recv-generator-acl-leak_test.py @@ -0,0 +1,68 @@ +#!/usr/bin/env python3 +"""KI-23: recv_generator leaks real_sx ACL data on the directory path. + +In generator.c:recv_generator the directory branch calls set_file_attrs() on +&real_sx, which (under --acls) loads the destination's current ACL into +real_sx via get_acl_fdat(), then does `goto cleanup`. The cleanup label only +frees sx, never real_sx (real_sx is freed solely on the regular-file path), so +each pre-existing ACL-bearing destination directory leaks its ACL data. + +ASan/LSan reproducer: sync --acls onto a destination directory that already +carries an ACL, then assert no leak report names recv_generator. Gated on an +AddressSanitizer build and on setfacl being available. +""" + +import glob +import os +import shutil +import subprocess + +from rsyncfns import ( + SCRATCHDIR, RSYNC, + acls_supported, require_asan, rmtree, rsync_argv, test_fail, test_skipped, +) + +require_asan("KI-23 real_sx ACL leak is only observable under AddressSanitizer/LSan", RSYNC) +if not acls_supported(): + test_skipped("rsync built without ACL support, or filesystem rejects ACLs") +if not shutil.which('setfacl'): + test_skipped("setfacl not available to plant a destination-directory ACL") + +base = SCRATCHDIR / 'acl-leak' +rmtree(base) +src = base / 'src' +dst = base / 'dst' +(src / 'sub').mkdir(parents=True) +(src / 'sub' / 'f.txt').write_text("updated-content\n") # different size -> always transferred +(dst / 'sub').mkdir(parents=True) +(dst / 'sub' / 'f.txt').write_text("old\n") + +# Plant an ACL on the pre-existing destination directory so recv_generator's +# directory branch loads it into real_sx (the leaked allocation). +if subprocess.run(['setfacl', '-m', 'u:nobody:rwx', str(dst / 'sub')]).returncode != 0: + test_skipped("setfacl could not set an ACL on the destination directory") + +asan_log = base / 'acl-leak-asan' +for stale in glob.glob(f"{asan_log}.*"): + os.unlink(stale) +os.environ['ASAN_OPTIONS'] = ( + f"detect_leaks=1:abort_on_error=0:log_path={asan_log}" +) + +p = subprocess.run(rsync_argv('-a', '--acls', f'{src}/', f'{dst}/'), + capture_output=True, text=True) + +# Non-vacuity: the transfer must have actually run through recv_generator's +# directory branch (the dest dir pre-exists with an ACL, and its child file is +# updated). We can't check the exit code: under detect_leaks=1 rsync's +# intentional at-exit leaks already force a nonzero status. +if (dst / 'sub' / 'f.txt').read_text() != "updated-content\n": + test_fail(f"--acls transfer did not update the destination; leak path not exercised:\n{p.stderr}") + +reports = ''.join(open(r, errors='replace').read() + for r in glob.glob(f"{asan_log}.*")) +if 'recv_generator' in reports: + test_fail("recv_generator leaked real_sx ACL data on the directory path (KI-23):\n" + + reports[:1500]) + +print("recv-generator-acl-leak: recv_generator does not leak real_sx ACL data") diff --git a/testsuite/uidlist-id0-name-leak_test.py b/testsuite/uidlist-id0-name-leak_test.py new file mode 100644 index 00000000..31bf2293 --- /dev/null +++ b/testsuite/uidlist-id0-name-leak_test.py @@ -0,0 +1,56 @@ +#!/usr/bin/env python3 +"""KI-25: send_one_list leaks the id-0 (root) name string. + +uidlist.c:send_one_list, when xmit_id0_names is negotiated, passes +uid_to_user(0)/gid_to_group(0) straight to send_one_name(); those return a +strdup'd name (uidlist.c:120/137) whose pointer is never stored or freed. Up +to two small strings leak per transfer. + +ASan/LSan reproducer: run an owner/group-preserving transfer (which sends the +uid+gid lists with the id-0 name) and assert no leak report names send_one_list. +Gated on an AddressSanitizer build. +""" + +import glob +import os +import subprocess + +from rsyncfns import ( + FROMDIR, SCRATCHDIR, RSYNC, + require_asan, rmtree, rsync_argv, test_fail, +) + +require_asan("KI-25 id-0 name leak is only observable under AddressSanitizer/LSan", RSYNC) + +src = FROMDIR +rmtree(src) +src.mkdir(parents=True) +(src / 'f.txt').write_text("hello\n") + +asan_log = SCRATCHDIR / 'id0-leak-asan' +for stale in glob.glob(f"{asan_log}.*"): + os.unlink(stale) +os.environ['ASAN_OPTIONS'] = ( + f"detect_leaks=1:abort_on_error=0:log_path={asan_log}" +) + +# -o/-g make the sender transmit the uid+gid lists; a modern<->modern transfer +# negotiates xmit_id0_names, so send_one_list emits the id-0 name. --list-only +# makes the client exit via exit() (so LSan runs) rather than the _exit() of the +# normal receiver/generator shutdown that would otherwise hide the leak. +p = subprocess.run(rsync_argv('-og', '--list-only', f'{src}/'), + capture_output=True, text=True) + +# Non-vacuity: the listing must have actually run (proving send_id_lists -> +# send_one_list executed). We can't check the exit code: under detect_leaks=1 +# rsync's intentional at-exit leaks already force a nonzero status. +if 'f.txt' not in p.stdout: + test_fail(f"--list-only did not list the source file; leak path not exercised:\n{p.stdout}{p.stderr}") + +reports = ''.join(open(r, errors='replace').read() + for r in glob.glob(f"{asan_log}.*")) +if 'send_one_list' in reports: + test_fail("send_one_list leaked the id-0 name string (KI-25):\n" + + reports[:1500]) + +print("uidlist-id0-name-leak: send_one_list does not leak the id-0 name") diff --git a/uidlist.c b/uidlist.c index 99a34679..5c1096ad 100644 --- a/uidlist.c +++ b/uidlist.c @@ -397,9 +397,11 @@ static void send_one_list(int f, struct idlist *idlist, int usernames) /* Terminate the uid list with 0 (which was excluded above). * A modern rsync also sends the name of id 0. */ - if (xmit_id0_names) - send_one_name(f, 0, usernames ? uid_to_user(0) : gid_to_group(0)); - else + if (xmit_id0_names) { + const char *name = usernames ? uid_to_user(0) : gid_to_group(0); + send_one_name(f, 0, name); + free((char *)name); + } else write_varint30(f, 0); } diff --git a/xattrs.c b/xattrs.c index d18236bd..926df17a 100644 --- a/xattrs.c +++ b/xattrs.c @@ -385,6 +385,7 @@ int copy_xattrs(const char *source, const char *dest, int dest_fd) "copy_xattrs: %ssetxattr(%s,\"%s\") failed", dest_fd >= 0 ? "f" : "l", full_fname(dest), name); errno = save_errno; + free(ptr); return -1; } free(ptr);