tlshd: three small fixes (printf format, strtok_r, ALPN count bound)

[warter666]

Hi @chucklever, re-opening here as requested in oracle#167 — same three fixes, three separate (independently revertable) commits, each with a DCO Signed-off-by:

1. tlshd: fix printf format for the certificate version — gnutls_x509_crt_get_version() returns an int; print with %d instead of %u. (oracle issue: tlshd/tags.c: printf format mismatch (%u with signed int) oracle/ktls-utils#166)

2. tlshd: use strtok_r() instead of strtok() — strtok() is not thread-safe; use the reentrant variant. (oracle issue: tlshd/quic.c: strtok() is not thread-safe; consider strtok_r() oracle/ktls-utils#164)

3. tlshd: bound the ALPN count parsed from the configuration — quic_session_set_alpns() wrote one stack-array entry per comma-separated token with no count check; a comma-heavy alpns setting overflows the fixed TLSHD_QUIC_MAX_ALPNS_LEN / 2 array. Configurations exceeding the array capacity are now rejected with a log message. (oracle issue: tlshd/quic.c: quic_session_set_alpns() has no bound check on the ALPN count oracle/ktls-utils#165)

[chucklever]

Thanks for re-posting these here. Comments per commit below, plus two
things that apply to all three.

All three commits

• The Fixes #164 / #165 / #166 trailers refer to issue numbers in
  the oracle repository. The same issues in this repository are tlshd/quic.c: strtok() is not thread-safe; consider strtok_r() #57
  (strtok), tlshd/quic.c: quic_session_set_alpns() has no bound check on the ALPN count #58 (ALPN bound), and tlshd/tags.c: printf format mismatch (%u with signed int) #59 (printf format). Please update
  the trailers so the merge closes the right issues.
• The DCO sign-off needs a real name. CONTRIBUTING.md asks for
  Signed-off-by: Your Name <you@example.org>, and the certification
  is made by a person, not a handle. A GitHub noreply address is fine
  if you prefer not to publish an email address.

tlshd: fix printf format for the certificate version

Looks good.

tlshd: use strtok_r() instead of strtok()

The change is fine, but the rationale in the commit message is not.
tlshd forks one child process per handshake and does not link
pthreads, so strtok()'s static state is never shared between
threads. The reason to prefer strtok_r() here is to avoid hidden
global state in a parser, not thread safety. Please reword the
message along those lines.

tlshd: bound the ALPN count parsed from the configuration

This one needs a different fix. The count check guards against a
symptom, and the real bug is elsewhere.

The kernel's QUIC_SOCKOPT_ALPN getter returns the comma-joined ALPN
string without a NUL terminator, and it may return up to
QUIC_ALPN_MAX_LEN (128) bytes. conn->alpns is exactly
TLSHD_QUIC_MAX_ALPNS_LEN (128) bytes. So an application that sets a
128-byte ALPN string fills the array completely, and
quic_session_set_alpns() then runs strtok_r() and strlen() off
the end of alpns[] into conn->ticket[]. When a session ticket is
present, that overwrites any comma byte in the ticket with NUL and
passes ticket bytes to GnuTLS as an ALPN token. When no ticket is
present, the zeroed struct happens to stop the scan.

With a properly terminated buffer, the new check cannot fire: 127
characters yield at most 64 tokens, which is the array size, and
GnuTLS rejects more than 8 protocols on its own.

Please fix the termination instead: make alpns one byte larger than
the kernel maximum and write a NUL at the length getsockopt()
returns. That also means the count check and its log message can go.
(As an aside, the message "Too many ALPNs configured" points at the
wrong place. The ALPN list comes from the application's socket option,
not from tlshd.conf.)

One related thing you may want to handle while in this function: an
interior empty token such as h3,,x survives the kernel's parser and
arrives here as an empty string, which then goes to GnuTLS as a
zero-length ALPN. RFC 7301 requires 1 to 255 octets. Skipping empty
tokens after the space-strip would close that. Optional for this PR,
but a separate commit if you take it.

[warter666]

All points addressed — the branch is rewritten as 4 commits (force-pushed):

• Trailers now point at this repository: Fixes #59 (printf format), Fixes #57 (strtok_r), Fixes #58 (ALPN termination).
• DCO sign-off is now Signed-off-by: Guancheng Wang <271496918+warter666@users.noreply.github.com> — real name, noreply address as you suggested.
• strtok_r commit message reworded along the lines you described: fork-per-handshake, no pthreads, so the motivation is avoiding hidden global state in a parser, not thread safety.
• Commit 3 replaced with the real fix: alpns[] is now one byte larger than the kernel maximum and quic_conn_get_config() writes a NUL at the length getsockopt() returns. The count check and its misdirected log message are gone.
• Took the optional suggestion as a separate commit 4: empty tokens (e.g. "h3,,x") are skipped after the space-strip, since RFC 7301 requires 1–255 octets.

Thanks for the detailed review — the unterminated-getsockopt analysis makes the failure mode much clearer than what I had.

[warter666]

Pushed a fix for the CI failures — they were mine: the conn->alpns[len] = '�'; line I added in commit 3 contained a literal NUL byte inside the character literal instead of the escape sequence, which -Werror rejects (quic.c:429: null character(s) preserved in literal). Commits 3 and 4 have been rewritten with the correct '�' escape; the earlier commits and the diff are unchanged otherwise. Sorry for the noise — CI caught it, which is what it's for.