fix(p6-iconv): prevent convert buffer overflow; validate both directions; reset on error; more tests

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';
 

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,10 +43,20 @@ 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)
+  if (!local_out || !remote_out)
+    return -1;
   *local_out = NULL;
-  if (remote_out)
   *remote_out = NULL;
   if (!spec || spec[0] == '\0')
     return -1;
@@ -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;
 }

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);

tests/integration/test_iconv.py

@@ -137,3 +137,114 @@ def test_iconv_garbage_spec_rejected(shared_server):
 
     result, _ = run_client(source, dest, flags=["--iconv=,,,"], port=shared_server.port)
     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))

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();