replace potentially unsafe C library calls flagged by OpenBSD linker

[rdmark]

The most significant change is in uam_random_string(): the srandom()/ random() fallback path is removed entirely. For an authentication nonce, silently degrading to a time-seeded deterministic PRNG is a security regression; the function now returns -1 if /dev/urandom cannot be opened. A follow-up fix also replaces the single read() with a loop that retries on EINTR and accumulates partial reads, ensuring the nonce buffer is always fully populated before returning to the caller.

Buffer safety in production code:

• mangle(): sprintf -> snprintf with sizeof(mangle_suffix); strcat ->
  strlcat(m, ..., MAXPATHLEN) for the mangle suffix and fallback "???"
• quota/getquota(): malloc+strcpy pairs replaced with strdup(); sprintf
  for the NFS quotas path replaced with snprintf(buf, sizeof(buf), ...)
• messages/readmessage(): malloc size captured in filenamelen, both
  sprintf calls replaced with snprintf(filename, filenamelen, ...)
• nad/ftw: strcpy in mystpcpy (already bounds-checked) -> strlcpy
• appl/afp_addappl and afp_rmvappl: strcpy(obj->oldtmp, dtf) ->
  strlcpy(..., AFPOBJ_TMPSIZ)
• desktop/setdeskmode and setdeskowner: strcpy/strcat into modbuf[13] ->
  strlcpy/strlcat with sizeof(modbuf)
• desktop/dtfile: strcat(path, ext) -> strlcat(..., sizeof(path))
• desktop/mtoupath: strcpy(upath, ".") -> strlcpy(..., sizeof(upath))

Test code brought to the same standard:

• afpfunc_helpers/cnamewrap: strcpy -> strlcpy(p, name, sizeof(buf)-len)
• afphelper/bitmap2text: all sprintf+strcat pairs -> snprintf+strlcat
• speedtest Write/Copy/ServerCopy/Read: sprintf -> snprintf, strcpy ->
  strlcpy, all with sizeof(temp)
• T2_FPByteRangeLock/test_bytelock: strcpy/strcat chain -> strlcpy/strlcat
• subtests/test001_add_x_dirs: sprintf -> snprintf
• T2_FPResolveID/test129: sprintf -> snprintf
• T2_FPGetFileDirParms/test336: sprintf -> snprintf;
  strcat -> strlcat(..., sizeof(temp))

afpd: use full appl buffer size AFPOBJ_TMPSIZ+1 in strlcpy

oldtmp is declared as char oldtmp[AFPOBJ_TMPSIZ + 1], but both strlcpy calls were passing AFPOBJ_TMPSIZ as the size limit, causing a path of exactly MAXPATHLEN bytes to be silently truncated and potentially directing open()/rename() at the wrong temp file.

messages MAXPATHLEN+1 stack buffer for message filename

sizeof(SERVERTEXT) + 15 left only six bytes for the PID decimal representation; a pid_t can require up to ten digits, causing snprintf to truncate and unlink() to operate on the wrong path.

Replace the dynamically-allocated buffer with a stack-local char filename[MAXPATHLEN + 1] and use sizeof(filename) in both snprintf calls, which also eliminates the malloc/free pair.

guard AppleDesktop strlcat truncation in setdeskmode and setdeskowner

modbuf[12+1] is correctly sized for the AppleDesktop format (2-char outer dir + "/" + up to 8-char hex-encoded creator code). However, strlcat silently truncated any entry whose name exceeded the remaining space, which would cause lstat/ochmod/chown to operate on an unintended path.

Check strlcat's return value and skip with a log warning if the name does not fit, matching the behaviour of the outer loop which already skips outer-directory names longer than two characters.

testsuite: fix afp helper length/data mismatch in cnamewrap

The advertised path length was taken from strlen(name) before strlcpy could truncate, producing a malformed AFP request where the uint16_t length field claimed more bytes than were actually present in the buffer.

AFP 3 (UTF-8) paths are length-prefixed, not null-terminated, so strlcpy was also the wrong primitive. Fix by:

• computing the available space after the length field header up front
• clamping namelen to that space before encoding it into the uint16_t field
• using memcpy to write exactly namelen bytes, keeping length and data
  in sync regardless of the input size

afpd: extract AppleDesktop desk_foreach to eliminate duplicated traversal

setdeskmode() and setdeskowner() shared identical two-level AppleDesktop directory traversal code, including the modbuf path-building logic and the strlcat overflow guard added in a prior fix.

Introduce desk_foreach(DIR *, desk_entry_fn, void *ctx) which handles
the iteration, and two static callbacks:

• deskmode_entry: lstat + ochmod (file vs dir mode bits)
• deskowner_entry: chown

Each public function now sets up its context struct, calls desk_foreach, and handles the root .AppleDesktop dir itself (which is not visited by the iterator).

afpd,testsuite: replace unsafe strcpy/sprintf with bounded equivalents

Replace remaining sprintf/strcpy/strcat calls flagged by the OpenBSD linker with snprintf/strlcpy/strlcat/memcpy throughout afpd and the test suite. Key fixes beyond mechanical substitution:

• dtfile(): build the creator/hex suffix in a fixed local buffer and
  append via strlcat, closing a buffer overflow in the *p++ writes
  that could follow a near-full snprintf result
• appl.c afp_addappl/afp_rmvappl: guard against NULL return from
  dtfile() before passing dtf to strlcpy; rmvappl also closes sdt_fd
  on this new error path to match existing cleanup
• file.c set_name: save strlen(name) into tp_len at the strdup site
  so the restore memcpy uses tp_len+1 instead of a redundant strlen

[augmentcode]

🤖 Augment PR Summary

Summary: This PR addresses OpenBSD fortified-toolchain warnings by replacing unsafe C library calls and tightening a security-sensitive nonce generator.

Changes:

• Remove the deterministic srandom()/random() fallback from uam_random_string(); now reads from /dev/urandom and fails if unavailable.
• Harden random-byte reading by looping until the requested nonce length is fully populated (with EINTR retry).
• Replace multiple strcpy/strcat/sprintf usages with bounded alternatives (strlcpy/strlcat/snprintf) in afpd code paths.
• Use strdup() instead of malloc()+strcpy() patterns in quota handling.
• Refactor AppleDesktop traversal in desktop.c into a shared iterator to apply chmod/chown safely across entries.
• Apply similar safe-string updates across the test suite (helpers, bitmap formatting, speed tests, byte-range lock tests).

Technical notes: Most changes are mechanical hardening; the primary behavioral change is that authentication nonce generation now fails fast rather than silently degrading to deterministic randomness.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. 1 suggestion posted.

Comment augment review to trigger a new review at any time.

[rdmark]

augment review

[augmentcode]

Review completed. 4 suggestions posted.

Comment augment review to trigger a new review at any time.

[rdmark]

augment review

[augmentcode]

Review completed. 2 suggestions posted.

Comment augment review to trigger a new review at any time.

[rdmark]

@andylemin FYI this became a slippery slope trying to appease the OpenBSD linker!

I'm thinking of breaking out the random string hardening and postpone the rest until another day and another stable release

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[github-actions]

🔥 Spectest (AFP 3.4) - Flamegraph (AFP_ASSERT active)

Commit: 8c44539a6a5c4c426470612468a9ab340e2edd54
Profiling: On-CPU sampling @ 1019 Hz (prime), DWARF call-graph, x86_64
Build: debugoptimized (-O2 -g -fno-omit-frame-pointer)
Total Runtime: 68s, Netatalk Code-time: 3.1%,
Stacks: 1095, SVG size: 664K

🔥 Open interactive Flamegraph (SVG)

📥 Download from artifacts →

🔝 Top 10 leaf functions

Function	Samples
_raw_spin_unlock_irqrestore	266928288
do_syscall_64	109911648
__cp_end	83415090
srso_alias_safe_ret	46123638
find_get_block_common	46123638
dircache_process_deferred_chain	27477912
xas_load	24533850
file_close_fd	22571142
tcp_sendmsg_locked	20608434
memcmp	20608434

[andylemin]

@andylemin FYI this became a slippery slope trying to appease the OpenBSD linker!

I'm thinking of breaking out the random string hardening and postpone the rest until another day and another stable release

Hi @rdmark thats absolutely reasonable. Not surprised it got out of hand quickly