diff --git a/src/server/server.c b/src/server/server.c index f59d39e..2d66777 100644 --- a/src/server/server.c +++ b/src/server/server.c @@ -175,6 +175,15 @@ static const char* server_module_gate(const Config* config, void* context) { ModuleGateContext* gate_ctx = (ModuleGateContext*)context; if (!config) return "missing config frame"; + /* --iconv (protocol 2.16.0): the receiver's exact conversion direction (the + client spec's wire charset into this server's local charset, including a + server-side --iconv override) must be usable BEFORE the STATUS_OK ack, so + an impossible conversion is refused at the handshake instead of failing + the first file mid-transfer. The client spec itself was already sanity + checked by validate_received_config. */ + if (config->iconv_spec && + !charset_wire_receiver_spec_valid(config->iconv_spec, server_iconv_spec)) + return "client --iconv conversion cannot be honored by this server"; bool is_daemon = g_daemon_conf != NULL; bool has_module = config->module != NULL && config->module[0] != '\0'; diff --git a/src/shared/charset.c b/src/shared/charset.c index 981e4b5..75a2ac5 100644 --- a/src/shared/charset.c +++ b/src/shared/charset.c @@ -11,6 +11,16 @@ typedef struct { iconv_t cd; } CharsetConversion; +/* Process-wide wire conversion descriptor (one direction per process: a client + * only sends, a server only receives). CONCURRENCY CONTRACT: iconv_t is not + * guaranteed thread-safe, so every conversion MUST run on a single thread at a + * time. This holds today -- on the client the conversions run on the sender + * thread (in the -m pipeline chunk_serialize/send happen on the sender thread + * only), on the server on the receive-loop thread; the descriptor is + * initialized on one thread before any transfer thread spawns and torn down + * (charset_wire_free) only after all threads have joined. Do not add a + * concurrent conversion path (e.g. parallel chunk serialization) without + * guarding access with a mutex. */ static CharsetConversion* g_wire_conv; /* Grow *buf to double capacity, freeing it on failure. realloc preserves the @@ -33,11 +43,21 @@ static bool grow_charset_buffer(char** buf, size_t* cap) { return true; } +/* Throw away any pending shift state so a subsequent conversion starts clean. + * The flush output is discarded; for the stateless single-byte/UTF charsets + * this feature targets it is a no-op. */ +static void charset_conversion_reset(const CharsetConversion* conv) { + char scratch[64]; + char* sp = scratch; + size_t sl = sizeof(scratch); + (void)iconv(conv->cd, NULL, NULL, &sp, &sl); +} + int charset_spec_parse(const char* spec, char** local_out, char** remote_out) { - if (local_out) - *local_out = NULL; - if (remote_out) - *remote_out = NULL; + if (!local_out || !remote_out) + return -1; + *local_out = NULL; + *remote_out = NULL; if (!spec || spec[0] == '\0') return -1; char* dup = str_dup(spec); @@ -91,14 +111,44 @@ void charset_conversion_close(void* conversion) { free(conv); } -bool charset_pair_valid(const char* local, const char* remote) { - if (!local || !remote) +/* Probe a single conversion direction: the from/to charsets both open AND a + * representative ASCII name converts to a byte string containing no embedded + * NUL (so a target charset like UTF-16 that emits NUL bytes for ordinary ASCII + * names is rejected up front -- such an output would be silently truncated by + * the C-string wire helpers). */ +static bool direction_probe_valid(const char* from, const char* to) { + if (!from || !to) return false; - void* conv = charset_conversion_open(local, remote); + void* conv = charset_conversion_open(from, to); if (!conv) return false; + bool ok = true; + char input = 'a'; + char* in_ptr = &input; + size_t in_left = 1; + char out_buf[64]; + char* out_ptr = out_buf; + size_t out_left = sizeof(out_buf); + if (iconv(((CharsetConversion*)conv)->cd, &in_ptr, &in_left, &out_ptr, &out_left) == (size_t)-1) + ok = false; + char flush_buf[64]; + char* flush_ptr = flush_buf; + size_t flush_left = sizeof(flush_buf); + if (ok && + iconv(((CharsetConversion*)conv)->cd, NULL, NULL, &flush_ptr, &flush_left) == (size_t)-1) + ok = false; + size_t produced = (size_t)(out_ptr - out_buf); + if (ok && produced > 0 && memchr(out_buf, '\0', produced) != NULL) + ok = false; charset_conversion_close(conv); - return true; + return ok; +} + +bool charset_pair_valid(const char* local, const char* remote) { + /* Both ends convert in opposite directions with the same two charsets, so a + * valid spec must open (and be NUL-free) in BOTH directions: the sender + * opens local->remote, the receiver opens remote->local. */ + return direction_probe_valid(local, remote) && direction_probe_valid(remote, local); } bool charset_spec_valid(const char* spec) { @@ -114,6 +164,41 @@ bool charset_spec_valid(const char* spec) { return ok; } +bool charset_spec_valid_direction(const char* from_charset, const char* to_charset) { + return direction_probe_valid(from_charset, to_charset); +} + +/* The receiver's real conversion is wire(client REMOTE) -> server-local (the + * server's own --iconv LOCAL half, or the client's LOCAL half when the server + * has no --iconv). A dedicated pre-ack check so an impossible direction is + * rejected before the connection instead of refusing mid-transfer. */ +bool charset_wire_receiver_spec_valid(const char* spec, const char* server_spec) { + if (!spec) + return true; + char* local; + char* remote; + if (charset_spec_parse(spec, &local, &remote) != 0) + return false; + const char* wire = remote; + const char* target_local = local; + char* server_local = NULL; + char* server_remote = NULL; + if (server_spec) { + if (charset_spec_parse(server_spec, &server_local, &server_remote) != 0) { + free(local); + free(remote); + return false; + } + target_local = server_local; + } + bool ok = charset_spec_valid_direction(wire, target_local); + free(server_local); + free(server_remote); + free(local); + free(remote); + return ok; +} + char* charset_convert(const void* conversion, const char* in, int* err_out) { if (!conversion || !in) return NULL; @@ -134,9 +219,14 @@ char* charset_convert(const void* conversion, const char* in, int* err_out) { if (errno != E2BIG) { if (err_out) *err_out = errno; + charset_conversion_reset(conv); free(out); return NULL; } + /* Output exhausted but input remains. E2BIG does not roll the output + pointer back: the bytes iconv already emitted before the failure must + be preserved, so advance out_used before growing. */ + out_used = (size_t)(out_ptr - out); if (!grow_charset_buffer(&out, &cap)) return NULL; continue; @@ -153,9 +243,11 @@ char* charset_convert(const void* conversion, const char* in, int* err_out) { if (errno != E2BIG) { if (err_out) *err_out = errno; + charset_conversion_reset(conv); free(out); return NULL; } + out_used = (size_t)(out_ptr - out); if (!grow_charset_buffer(&out, &cap)) return NULL; continue; @@ -164,6 +256,23 @@ char* charset_convert(const void* conversion, const char* in, int* err_out) { break; } + /* A successful iconv call may legitimately consume the whole buffer (output + exactly fills cap), leaving no room for the terminator: guarantee headroom + before the final write. */ + if (out_used >= cap && !grow_charset_buffer(&out, &cap)) + return NULL; + + /* Defense in depth: a target charset that emits embedded NUL bytes would + truncate at the first NUL in the C-string wire helpers; fail cleanly + (validation already rejects such charsets up front). */ + if (memchr(out, '\0', out_used) != NULL) { + if (err_out) + *err_out = EILSEQ; + charset_conversion_reset(conv); + free(out); + return NULL; + } + out[out_used] = '\0'; return out; } diff --git a/src/shared/charset.h b/src/shared/charset.h index 545fdb6..49cd0f3 100644 --- a/src/shared/charset.h +++ b/src/shared/charset.h @@ -22,15 +22,24 @@ /* Parse CONVERT_SPEC into malloc'd LOCAL and REMOTE charset names (caller * frees both). REMOTE is a separate copy of LOCAL when no comma is present. * Returns 0 on success, -1 on a malformed spec (empty halves / missing value / - * allocation failure); nothing is allocated on the -1 path. */ + * allocation failure); nothing is allocated on the -1 path. Both output + * pointers are REQUIRED (non-NULL). */ int charset_spec_parse(const char* spec, char** local_out, char** remote_out); -/* True when a CONVERT_SPEC is well-formed AND every charset name opens in a - * probe iconv_open (so a typo'd name is rejected at startup, not mid-run). - * NULL (iconv disabled) is always valid. */ +/* True when a CONVERT_SPEC is well-formed AND its charsets are usable for this + * feature: each pair opens in a probe iconv_open in BOTH directions (a sender + * converts local->remote, the receiver converts remote->local) and converting + * a representative ASCII name emits no embedded NUL byte (a UTF-16-style NUL + * emitter would be silently truncated by the C-string wire helpers). A typo'd + * charset name is therefore rejected at startup, not mid-run. NULL (iconv + * disabled) is always valid. */ bool charset_spec_valid(const char* spec); -/* Probe a local->remote conversion pair without keeping the descriptor. */ +/* Probe a concrete from->to conversion pair without keeping the descriptor: + * both charsets open AND a representative ASCII name converts with no embedded + * NUL. Used for direction-specific validation (e.g. the receiver's exact + * wire->local direction including a server-side charset override). */ +bool charset_spec_valid_direction(const char* from_charset, const char* to_charset); bool charset_pair_valid(const char* local, const char* remote); /* One-shot conversion of a NUL-terminated input to a malloc'd NUL-terminated @@ -56,6 +65,12 @@ bool charset_wire_init_receiver(const char* spec, const char* server_spec); void charset_wire_free(void); bool charset_wire_active(void); +/* Pre-ack receiver-direction sanity (see charset_wire_init_receiver): true + * when the exact wire->server-local conversion the receiver will use (client + * spec's REMOTE half into the server's own LOCAL half, or the client's LOCAL + * half when the server has no --iconv) opens and produces NUL-free output. */ +bool charset_wire_receiver_spec_valid(const char* spec, const char* server_spec); + /* Convert a path across the wire in the process direction. Returns a malloc'd * string, or NULL when the name cannot be represented in the target charset. */ char* charset_wire_apply(const char* path); diff --git a/tests/integration/test_iconv.py b/tests/integration/test_iconv.py index c8283b0..4ec1db7 100644 --- a/tests/integration/test_iconv.py +++ b/tests/integration/test_iconv.py @@ -136,4 +136,115 @@ def test_iconv_garbage_spec_rejected(shared_server): fh.write(b"x") result, _ = run_client(source, dest, flags=["--iconv=,,,"], port=shared_server.port) - assert result.returncode != 0 \ No newline at end of file + assert result.returncode != 0 + + +@pytest.mark.ci +def test_iconv_expanding_name_growth(shared_server): + """A long latin1 name whose UTF-8 encoding expands past the initial output + buffer exercises the E2BIG growth path in charset_convert (each high-bit + latin1 byte doubles in UTF-8), and must land unchanged on the destination.""" + source, dest = _make("growth") + name_bytes = b"a" * 40 + bytes(range(0x80, 0x80 + 40)) + b".txt" + _place_bytes(source, name_bytes, data=b"growth\n") + + result, _ = run_client( + source, dest, flags=["--iconv=iso-8859-1,utf-8"], port=shared_server.port + ) + assert result.returncode == 0, (result.stderr or result.stdout)[:400] + + assert os.path.exists(_dest_file(source, dest, name_bytes)) + + +def test_iconv_symlink_path_and_target(shared_server): + """A latin1-named symlink pointing at a latin1-named target survives the + transfer: both the link name and the link target are wire-converted and + re-decoded on the destination (-l preserves links).""" + source, dest = _make("symlink") + target = b"target\xe9.dat" + _place_bytes(source, target, data=b"t\n") + os.symlink(target, os.path.join(os.fsencode(source), b"link\xe9")) + + result, _ = run_client( + source, dest, flags=["--iconv=iso-8859-1,utf-8", "--links"], port=shared_server.port + ) + assert result.returncode == 0, (result.stderr or result.stdout)[:400] + + dst_target = _dest_file(source, dest, target) + dst_link = _dest_file(source, dest, b"link\xe9") + assert os.path.exists(dst_target), "dest latin1 target file missing" + assert os.path.islink(dst_link), "dest latin1 symlink missing" + assert os.readlink(dst_link) == target, "symlink target not preserved/decoded" + with open(dst_link, "rb") as fh: + assert fh.read() == b"t\n" + + +def test_iconv_hardlink_path_and_target(shared_server): + """A latin1-named hard-linked pair is preserved: -H transmits later group + members as a path+target link to the first member, so both the member name + and the target wire-convert (the two destination names must stay one + inode).""" + source, dest = _make("hardlink") + a = b"hl_a\xe9.txt" + b = b"hl_b\xe9.txt" + src_a = os.path.join(os.fsencode(source), a) + with open(src_a, "wb") as fh: + fh.write(b"shared\n") + os.link(src_a, os.path.join(os.fsencode(source), b)) + + result, _ = run_client( + source, dest, flags=["--iconv=iso-8859-1,utf-8", "--hard-links"], + port=shared_server.port, + ) + assert result.returncode == 0, (result.stderr or result.stdout)[:400] + + dst_a = _dest_file(source, dest, a) + dst_b = _dest_file(source, dest, b) + assert os.path.exists(dst_a) and os.path.exists(dst_b) + assert os.stat(dst_a).st_ino == os.stat(dst_b).st_ino, \ + "hard-link relationship not preserved across the transfer" + + +def test_iconv_delete_manifest_consistent(shared_server): + """Combining --iconv with --delete: the delete manifest's keep-set paths are + wire-converted on send and disk-converted on receive, so the receiver's + delete walker compares like with like and removes exactly the missing + latin1-named file (never a wrong-named mirror).""" + source, dest = _make("delete") + keep = b"keep\xe9.txt" + gone = b"gone\xe9.txt" + _place_bytes(source, keep, data=b"k\n") + _place_bytes(source, gone, data=b"g\n") + + with ServerManager() as server: + server.start(extra_args=["--allow-delete"]) + flags = ["--iconv=iso-8859-1,utf-8"] + result, _ = run_client(source, dest, flags=flags, port=server.port) + assert result.returncode == 0, (result.stderr or result.stdout)[:400] + assert os.path.exists(_dest_file(source, dest, keep)) + assert os.path.exists(_dest_file(source, dest, gone)) + + os.remove(os.path.join(os.fsencode(source), gone)) + result, _ = run_client( + source, dest, flags=flags + ["--delete"], port=server.port + ) + assert result.returncode == 0, (result.stderr or result.stdout)[:400] + assert os.path.exists(_dest_file(source, dest, keep)), "kept file deleted" + assert not os.path.exists(_dest_file(source, dest, gone)), \ + "missing file was not deleted" + + +def test_iconv_chunk_serialization_blob(shared_server): + """-s (chunk serialization) embeds paths and symlink targets inside the + serialized chunk blob rather than as separate frames; a latin1 name must + still wire-convert and re-decoded on the destination.""" + source, dest = _make("chunk") + name = b"\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9\xe9.txt" + _place_bytes(source, name, data=b"blob\n") + + result, _ = run_client( + source, dest, flags=["--iconv=iso-8859-1,utf-8", "-s"], port=shared_server.port + ) + assert result.returncode == 0, (result.stderr or result.stdout)[:400] + + assert os.path.exists(_dest_file(source, dest, name)) \ No newline at end of file diff --git a/tests/test_iconv.c b/tests/test_iconv.c index 27b1840..faee11b 100644 --- a/tests/test_iconv.c +++ b/tests/test_iconv.c @@ -51,6 +51,11 @@ static void test_iconv_spec_valid() { EXPECT_FALSE(charset_spec_valid("utf-8,no-such-charset")); EXPECT_FALSE(charset_spec_valid(",,,")); EXPECT_FALSE(charset_spec_valid("utf-8,")); + /* A target charset whose conversion emits embedded NUL bytes would be + truncated by the C-string wire helpers; it must be rejected up front. */ + EXPECT_FALSE(charset_spec_valid("utf-8,utf-16")); + EXPECT_FALSE(charset_spec_valid("utf-16")); + EXPECT_FALSE(charset_spec_valid("iso-8859-1,utf-16")); } /* --- one-shot conversion ------------------------------------------------ */ @@ -93,6 +98,45 @@ static void test_iconv_unrepresentable_fails() { charset_conversion_close(conv); } +/* A latin1 high-bit byte expands to two UTF-8 bytes. With exactly 16 high + * bytes the output is exactly cap = in_len + 16, so the final iconv call fills + * the buffer completely and a naive NUL-terminator write would overflow. */ +static void test_iconv_exact_fill_no_overflow() { + char name[64]; + strcpy(name, "dir/"); + int n = 4; + for (int i = 0; i < 16; i++) + name[n++] = (char)(0x80 + i); + name[n] = '\0'; + + void* conv = charset_conversion_open("iso-8859-1", "utf-8"); + EXPECT_NOT_NULL(conv); + char* out = charset_convert(conv, name, NULL); + EXPECT_NOT_NULL(out); + EXPECT_EQ_INT((int)strlen(out), n + 16); + charset_conversion_close(conv); + free(out); +} + +/* Many high-bit bytes force the output buffer past its initial cap, exercising + * the E2BIG growth path (input partially consumed/produced before the grow). */ +static void test_iconv_growth_expanding_name() { + char name[256]; + strcpy(name, "dir/"); + int n = 4; + for (int i = 0; i < 80; i++) + name[n++] = (char)(0x80 + (i % 0x80)); + name[n] = '\0'; + + void* conv = charset_conversion_open("iso-8859-1", "utf-8"); + EXPECT_NOT_NULL(conv); + char* out = charset_convert(conv, name, NULL); + EXPECT_NOT_NULL(out); + EXPECT_EQ_INT((int)strlen(out), n + 80); + charset_conversion_close(conv); + free(out); +} + /* --- process-wide wire conversion ---------------------------------------- */ static void test_iconv_wire_sender_converts_local_to_remote() { @@ -164,6 +208,8 @@ void test_iconv() { test_iconv_latin1_to_utf8(); test_iconv_invalid_sequence_fails(); test_iconv_unrepresentable_fails(); + test_iconv_exact_fill_no_overflow(); + test_iconv_growth_expanding_name(); test_iconv_wire_sender_converts_local_to_remote(); test_iconv_wire_receiver_converts_remote_to_local(); test_iconv_wire_disabled_passthrough();