Fix deadlock in syncLocal when receiver fails partway

.github/workflows/ci.yaml

@@ -75,12 +75,15 @@ jobs:
       # windows-latest's own default toolchain without access to a real
       # GitHub Actions runner to test against - so -race runs where it's
       # known-good (Linux), and windows-latest still gets a full,
-      # real, native (not cross-compiled) test run without it. The
-      # actual value -race provides - catching a genuine data race in
-      # this project's own logic (SC-10's progress-reporter goroutine,
-      # SC-6's per-connection daemon handling) - isn't platform-specific
-      # in what it would find, so one platform running it is enough to
-      # get that benefit.
+      # real, native (not cross-compiled) test run without it. This
+      # isn't just checkbox coverage: the first real CI run of this leg
+      # caught a genuine deadlock in syncLocal (internal/cli/sync.go) -
+      # a receiver failure partway through a sync left the sender
+      # goroutine parked forever reading a reply that would never come,
+      # timing out the whole test binary after 10 minutes. The bug ran
+      # fine every time on Windows without -race (the race window never
+      # opened), which is exactly why relying on local, non-race runs
+      # alone would never have caught it.
       - name: go test -race (Linux)
         if: runner.os == 'Linux'
         run: go test -race -timeout 10m ./...

README.md

@@ -1589,14 +1589,21 @@ matters as much as its size for real throughput.
 Windows runners. `-race` runs on the Linux leg only: it requires cgo and
 a C toolchain, reliably available on `ubuntu-latest` but not something
 this project can verify for `windows-latest` without a real runner to
-test against, and the races it would actually catch (SC-10's
-progress-reporter goroutine, SC-6's per-connection daemon handling)
-aren't platform-specific in nature, so one reliable platform is enough.
-The Linux leg also installs a real `rsync` binary so the comparison tests
-above get a genuine execution somewhere in CI, not just a proof they can
-skip; the Windows leg has no equivalent easy install and instead
-exercises the graceful-skip path, matching a real Windows dev machine
-without rsync.
+test against. This isn't theoretical - the first real CI run of this leg
+caught a genuine deadlock in `syncLocal` (`internal/cli/sync.go`): a
+receiver failure partway through a sync left the sender goroutine parked
+forever reading a reply that would never come, timing out the whole test
+binary after 10 minutes. The same test passed instantly, every time, on
+Windows without `-race` - the race window only opened under `-race`'s own
+added scheduling overhead - which is exactly why local, non-race runs
+alone would never have caught it. (Fixed by having both `syncLocal`
+goroutines close the pipe halves they own, with `CloseWithError`, once
+they're done - see the function's own comments for the full
+explanation.) The Linux leg also installs a real `rsync` binary so the
+comparison tests above get a genuine execution somewhere in CI, not just
+a proof they can skip; the Windows leg has no equivalent easy install and
+instead exercises the graceful-skip path, matching a real Windows dev
+machine without rsync.
 
 ## Architecture
 

internal/daemon/auth.go

@@ -14,63 +14,36 @@ import (
 	"golang.org/x/crypto/md4"
 )
 
-// ErrAuthFailed is returned by ServeAuth and DialAuth when authentication
-// is attempted and fails - a wrong password, an unauthorized user, or a
-// secrets file that can't be read. The wire-level detail behind it is
-// deliberately generic (see ServeAuth), matching real rsync's own refusal
-// to distinguish "no such user" from "wrong password" in its response.
+// ErrAuthFailed is returned by ServeAuth and DialAuth when authentication fails.
 var ErrAuthFailed = errors.New("authentication failed")
 
-// AuthRequired reports whether a client must authenticate to use m: real
-// rsyncd.conf's own rule is that a module requires auth exactly when it
-// has a non-empty "auth users" list.
+// AuthRequired reports whether a client must authenticate to use m.
 func (m Module) AuthRequired() bool {
 	return len(m.AuthUsers) > 0
 }
 
 // PasswordFunc resolves the password to use for daemon authentication.
-// DialAuth calls it at most once, and only if the server actually
-// challenges for a password (an "@RSYNCD: AUTHREQD" line) - matching real
-// rsync's own client behavior, where auth_client() is only ever invoked
-// in response to that same line. Connecting to a module that turns out
-// not to require authentication never calls this at all, so a caller
-// backing it with an interactive terminal prompt or a --password-file
-// read never triggers either one against an anonymous module - resolving
-// eagerly, before knowing whether the server will ask, would be a real
-// regression from real rsync's behavior here, not just a style choice.
+// DialAuth calls it at most once, and only if the server sends an
+// "@RSYNCD: AUTHREQD" challenge; a module that turns out not to require
+// auth never triggers a call at all.
 type PasswordFunc func() (string, error)
 
-// StaticPassword wraps an already-known password (e.g. a test fixture, or
-// a caller that has already decided eager resolution is fine) as a
-// PasswordFunc.
+// StaticPassword wraps an already-known password as a PasswordFunc.
 func StaticPassword(password string) PasswordFunc {
 	return func() (string, error) { return password, nil }
 }
 
-// md4Hash returns the base64-encoded (standard alphabet, no padding - the
-// same encoding real rsync's own base64_encode(..., pad=0) produces) MD4
-// digest of secret followed by challenge. This matches real rsync's
-// generate_hash() in authenticate.c exactly: the secret is hashed first,
-// then the challenge, with no seed byte - verified against the actual
-// rsync source rather than assumed, since getting the byte order wrong
-// here would silently produce a client and server that only interoperate
-// with each other, never with real rsync or real docs describing the
-// algorithm.
+// md4Hash returns the base64-encoded MD4 digest of secret followed by
+// challenge, matching real rsync's generate_hash(): secret then challenge,
+// no seed byte.
 func md4Hash(secret, challenge string) string {
 	h := md4.New()
-	// hash.Hash.Write (which io.WriteString goes through) never returns
-	// an error - its doc comment guarantees this - so there is nothing
-	// meaningful to check here.
 	_, _ = io.WriteString(h, secret)
 	_, _ = io.WriteString(h, challenge)
 	return base64.RawStdEncoding.EncodeToString(h.Sum(nil))
 }
 
 // generateChallenge returns a fresh, random, base64-encoded challenge.
-// Real rsync derives its challenge from the client address, current time,
-// and pid; grsync uses a CSPRNG instead, which is at least as
-// unpredictable and far simpler - nothing in the protocol requires the
-// challenge to be derived any particular way, only that it not repeat.
 func generateChallenge() (string, error) {
 	buf := make([]byte, 16)
 	if _, err := rand.Read(buf); err != nil {
@@ -79,10 +52,7 @@ func generateChallenge() (string, error) {
 	return base64.RawStdEncoding.EncodeToString(buf), nil
 }
 
-// readSecretsFile parses a "name:secret" per-line secrets file, matching
-// the real "secrets file" format. Blank lines and "#"-prefixed lines are
-// skipped; a line with no ":" is skipped rather than treated as an error,
-// since it can never match a submitted username anyway.
+// readSecretsFile parses a "name:secret" per-line secrets file.
 func readSecretsFile(path string) (map[string]string, error) {
 	f, err := os.Open(path)
 	if err != nil {
@@ -118,16 +88,10 @@ func userAllowed(user string, allowed []string) bool {
 	return false
 }
 
-// ServeAuth runs the server side of module authentication, following
-// ServeGreeting having already selected m. If m doesn't require auth, it
-// writes "@RSYNCD: OK" and returns immediately with an empty user. Password
-// comparison happens in constant time (crypto/subtle) so a wrong-length or
-// wrong-content response can't be distinguished by timing; the refusal
-// reason is likewise never revealed on the wire, matching real rsync's own
-// single generic "@ERROR: auth failed on module <name>" message regardless
-// of whether the username was unknown, unauthorized, or the password was
-// wrong - only the server's own logs (not implemented here) would ever see
-// that detail in real rsync.
+// ServeAuth runs the server side of module authentication for m, which
+// ServeGreeting has already selected. Password comparison is constant-time
+// and the failure reason is never revealed on the wire, matching real
+// rsync's single generic auth-failure message.
 func ServeAuth(c *conn, m Module) (user string, err error) {
 	if !m.AuthRequired() {
 		if err := writeLine(c.w, "@RSYNCD: OK"); err != nil {
@@ -181,15 +145,10 @@ func ServeAuth(c *conn, m Module) (user string, err error) {
 	return submittedUser, nil
 }
 
-// DialAuth runs the client side of authentication: reads lines until
-// "@RSYNCD: OK", answering an "@RSYNCD: AUTHREQD <challenge>" line (if one
-// arrives) with "<user> <md4Hash(password, challenge)>" - the password
-// itself is never sent, only this one-way hash of it, so ErrAuthFailed
-// and any wire capture of this exchange should never contain it. An empty
-// user is sent as "nobody", matching real rsync's own client behavior for
-// anonymous-looking auth attempts. password is only ever called if the
-// server actually asks for one - see PasswordFunc's doc comment for why
-// that laziness matters, not just how it works.
+// DialAuth runs the client side of authentication: it reads lines until
+// "@RSYNCD: OK", answering any "@RSYNCD: AUTHREQD <challenge>" with
+// "<user> <md4Hash(password, challenge)>". The password itself is never
+// sent on the wire. An empty user is sent as "nobody".
 func DialAuth(c *conn, user string, password PasswordFunc) error {
 	for {
 		line, err := readLine(c.r)

internal/daemon/auth_test.go

@@ -20,8 +20,7 @@ func writeSecretsFile(t *testing.T, content string) string {
 }
 
 // authPipe wires up a client/server conn pair over io.Pipe, each side's
-// writes also captured into its own log buffer - so a test can inspect
-// every byte either side put on the wire, not just the final result.
+// writes also captured into its own log buffer.
 func authPipe() (client, server *conn, clientWireLog, serverWireLog *bytes.Buffer) {
 	clientR, serverW := io.Pipe()
 	serverR, clientW := io.Pipe()
@@ -117,10 +116,8 @@ func TestAuth_UnauthorizedUserFails(t *testing.T) {
 	}
 }
 
-// TestAuth_NoPlaintextPasswordOnWire is the self-review requirement made
-// concrete: it inspects the actual bytes each side wrote, not just the
-// outcome, and fails if the raw password ever appears in either
-// direction.
+// TestAuth_NoPlaintextPasswordOnWire fails if the raw password ever
+// appears in either side's wire bytes.
 func TestAuth_NoPlaintextPasswordOnWire(t *testing.T) {
 	const password = "correct-horse-battery-staple"
 	secretsPath := writeSecretsFile(t, "alice:"+password+"\n")

internal/daemon/client.go

@@ -9,20 +9,10 @@ import (
 )
 
 // DialClient runs a full client-side daemon session over an already-
-// connected nc: the greeting/module-selection handshake, authentication
-// (only actually triggered if the server challenges for it - see
-// PasswordFunc), and the resulting transfer against localPath. It is the
-// client-side counterpart to ServeConn, and exists for the same reason:
-// DialGreeting/DialAuth/DialModule are built around this package's own
-// unexported conn type, so external callers (internal/cli, in
-// particular) had no way to actually reach them until now - this is
-// where net.Conn crosses into that internal representation.
-//
-// module must be non-empty: DialClient runs a transfer, not a listing -
+// connected nc: the greeting/module-selection handshake, authentication,
+// and the resulting transfer against localPath. module must be non-empty;
 // callers that want to list a daemon's modules should use DialGreeting
-// directly with an empty module instead. See DialModule's own doc
-// comment for exactly what ropts and copts do and don't reach on each
-// direction.
+// directly with an empty module instead.
 func DialClient(nc net.Conn, module, user string, password PasswordFunc, direction Direction, localPath string, rules []sync.Rule, walkOpts sync.WalkOptions, attrOpts sync.AttrOptions, ropts pipeline.ReceiverOptions, copts pipeline.CompressOptions) error {
 	if module == "" {
 		return fmt.Errorf("DialClient requires a module name")

internal/daemon/client_test.go

@@ -83,12 +83,9 @@ func TestDialClient_UploadOverRealTCP(t *testing.T) {
 	}
 }
 
-// TestDialClient_PasswordFuncNotCalledForAnonymousModule is the laziness
-// guarantee made concrete: PasswordFunc must never be invoked when the
-// server never challenges for a password, exactly matching real rsync's
-// own auth_client(), which is only ever called in response to an
-// AUTHREQD line. A PasswordFunc backed by an interactive terminal prompt
-// or a --password-file read must not fire against an anonymous module.
+// TestDialClient_PasswordFuncNotCalledForAnonymousModule confirms
+// PasswordFunc is never invoked when the server doesn't challenge for a
+// password (an anonymous module).
 func TestDialClient_PasswordFuncNotCalledForAnonymousModule(t *testing.T) {
 	modRoot := t.TempDir()
 	mustWriteFile(t, filepath.Join(modRoot, "open.txt"), "no auth needed")

internal/daemon/config.go

@@ -1,10 +1,8 @@
 // Package daemon implements grsync's rsync-daemon-protocol server: parsing
 // rsyncd.conf, the rsync:// URL scheme, the @RSYNCD greeting/handshake,
 // MD4 challenge-response authentication, and per-module access control.
-// Once a client has authenticated and selected a module, this package
-// hands the connection straight to internal/pipeline's existing
-// Sender/Receiver - the daemon protocol is a second way to *establish* a
-// connection, not a second way to *transfer files*.
+// Once a client has authenticated and selected a module, the connection is
+// handed to internal/pipeline's Sender/Receiver for the actual transfer.
 package daemon
 
 import (
@@ -19,31 +17,24 @@ import (
 // whatever global defaults it didn't override.
 type Module struct {
 	Name string
-	// Path is the directory on the daemon's filesystem this module
-	// exposes. Required for every module - rsyncd.conf itself requires it.
+	// Path is the directory on the daemon's filesystem this module exposes.
 	Path string
-	// ReadOnly defaults to true, matching real rsync: "The default is for
-	// all modules to be read only."
+	// ReadOnly defaults to true, matching real rsync.
 	ReadOnly bool
-	// List defaults to true; "list = false" hides the module from a
-	// #list request without preventing a client who already knows its
-	// name from connecting to it - a real, documented rsyncd.conf option,
-	// not an invented one.
+	// List defaults to true; false hides the module from a #list request
+	// without preventing a client who already knows its name from connecting.
 	List bool
-	// Comment is shown alongside the module name in a #list response
-	// ("<name>\t<comment>", real rsync's own listing format).
+	// Comment is shown alongside the module name in a #list response.
 	Comment string
 	// Exclude is the raw, space-separated pattern list from the "exclude"
-	// parameter, not yet compiled into sync.Rule - see access.go.
+	// parameter, not yet compiled into sync.Rule.
 	Exclude []string
 	// AuthUsers is the raw, comma/space-separated list from "auth users".
 	// A non-empty list means this module requires authentication.
 	AuthUsers []string
-	// SecretsFile is the path to a "name:password" per-line file, per
-	// "secrets file".
+	// SecretsFile is the path to a "name:password" per-line file.
 	SecretsFile string
-	// MaxConnections is the simultaneous-connection cap for this module;
-	// 0 (the default) means unlimited, matching real rsync.
+	// MaxConnections is the simultaneous-connection cap; 0 means unlimited.
 	MaxConnections int
 }
 
@@ -52,33 +43,14 @@ type Config struct {
 	Modules map[string]Module
 }
 
-// moduleDefaults returns the built-in defaults every module starts from
-// before its own [section] parameters (or the file's global parameters,
-// set before any module header) are applied on top.
 func moduleDefaults() Module {
 	return Module{ReadOnly: true, List: true}
 }
 
-// ParseConfig parses rsyncd.conf content from r.
-//
-// Syntax, matching the real format (verified against the actual
-// rsyncd.conf(5) man page, not assumed): global parameters may appear
-// before any module header and become that module's starting defaults;
-// a module begins with "[name]" and continues until the next module or
-// EOF; "#"-prefixed lines are comments; blank lines are ignored; a line
-// ending in "\" continues on the next line; only the first "=" in a
-// "name = value" line is significant, and whitespace around it is
-// trimmed.
-//
-// A parameter name this package doesn't implement (real rsyncd.conf has
-// dozens - "uid", "hosts allow", "log file", "timeout", and more) is
-// accepted and silently ignored, not an error: rejecting a real,
-// syntactically valid config file just because it uses an option this
-// package hasn't implemented yet would be worse than ignoring that one
-// line. A line that isn't valid "name = value" or "[section]" syntax at
-// all, or a recognized parameter with a malformed value (e.g.
-// "max connections = abc"), is a hard parse error - that distinction is
-// deliberate, not an oversight.
+// ParseConfig parses rsyncd.conf content from r. Global parameters before
+// any module header become that module's starting defaults. An
+// unrecognized parameter name is silently ignored; a malformed line or a
+// recognized parameter with an invalid value is a hard error.
 func ParseConfig(r io.Reader) (*Config, error) {
 	lines, err := readLogicalLines(r)
 	if err != nil {

internal/daemon/fuzz_test.go

@@ -6,16 +6,7 @@ import (
 	"testing"
 )
 
-// FuzzReadGreeting is SC-15's fuzz target for the daemon protocol's own
-// greeting-line parsing (the "@RSYNCD: VERSION.SUB ..." line every
-// connection starts with, before any authentication happens - genuinely
-// untrusted network input, arriving before the peer has proven anything
-// about itself). The property checked is that readGreeting never panics
-// for any line content, however malformed - its own prefix check,
-// strings.Fields split, and two strconv.Atoi calls all look
-// defensively coded already, but fuzzing confirms that holds for inputs
-// nobody thought to hand-write, not just the cases
-// TestReadGreeting-style unit tests already cover.
+// FuzzReadGreeting checks that readGreeting never panics on malformed input.
 func FuzzReadGreeting(f *testing.F) {
 	f.Add("@RSYNCD: 31.0\n")
 	f.Add("@RSYNCD: 30\n")
@@ -28,24 +19,12 @@ func FuzzReadGreeting(f *testing.F) {
 
 	f.Fuzz(func(_ *testing.T, line string) {
 		r := bufio.NewReader(strings.NewReader(line))
-		// readGreeting itself calls readLine, so a line with no trailing
-		// "\n" is a valid, expected input here too (readLine returns an
-		// error for it) - not appended manually, so this fuzzes the real
-		// end-to-end parsing path exactly as a live connection would
-		// present it.
 		_, _, _ = readGreeting(r)
 	})
 }
 
-// FuzzReadLine is SC-15's fuzz target for the single shared line-reading
-// primitive every text-based phase of the daemon protocol (greeting,
-// module selection, authentication) reads through - the one chokepoint
-// genuinely untrusted network bytes always pass. The property checked is
-// exactly what readLine's own doc comment promises: it never panics, and
-// it never returns a line longer than maxLineLength, regardless of how
-// much unterminated data a hostile or corrupted peer sends - the
-// protection that keeps an attacker from forcing unbounded memory growth
-// just by never sending a newline.
+// FuzzReadLine checks that readLine never panics and never returns a line
+// longer than maxLineLength.
 func FuzzReadLine(f *testing.F) {
 	f.Add("hello\n")
 	f.Add("\n")