Re: [PATCH] tlshd: Return errnos from QUIC helper functions

[Chuck Lever <chuck.lever@oracle.com>]

On 9/23/25 3:41 PM, Xin Long wrote:
> On Tue, Sep 23, 2025 at 5:02 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>
>> On 9/23/25 1:27 PM, Xin Long wrote:
>>> On Tue, Sep 23, 2025 at 3:18 PM Chuck Lever <chuck.lever@oracle.com> wrote:
>>>>
>>>> On 9/22/25 3:07 PM, Chuck Lever wrote:
>>>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>>>
>>>>> Static analysis tools have noticed that
>>>>> tlshd_quic_serverhello_handshake() expects its helper functions to
>>>>> return errno values, but two of them return GNUTLS_E_* values in
>>>>> most cases. Convert those functions to always return errno values
>>>>> or zero (on success).
>>>>>
>>>>> Cc: Xin Long <lucien.xin@gmail.com>
>>>>> Fixes: 43a15fed2f33 ("tlshd: add support for quic handshake")
>>>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>>>> ---
>>>>>  src/tlshd/server.c | 54 +++++++++++++++++++++++++++-------------------
>>>>>  1 file changed, 32 insertions(+), 22 deletions(-)
>>>>
>>>> Xin -
>>>>
>>>> Looks like src/tlshd/quic.c has the same issue with conn->errcode.
>>>> Callers expect an errno, but internally the utility functions set that
>>>> field to a GNUTLS_E_ value.
>>>>
>>>> Can you apply this patch, fix up quic.c as well, and then test both and
>>>> post the fixes here?
>>>>
>>> Sure.
>>>
>>> But conn->errcode is used to pass the err to parms->session_status.
>>> What does parms->session_status expect? errno or GNUTLS_E_ value?
>>
>> session_status contains a positive errno or zero.
>>
>>
>>> looking at tlshd_start_tls_handshake():
>>>                 /* Any errors here should default to blocking access: */
>>>                 parms->session_status = EACCES; <---
>>>                 switch (ret) {
>>>                 case GNUTLS_E_CERTIFICATE_ERROR:
>>>                 case GNUTLS_E_CERTIFICATE_VERIFICATION_ERROR:
>>>                         tlshd_log_cert_verification_error(session);
>>>                         break;
>>>                 case -ETIMEDOUT:
>>>                         tlshd_log_gnutls_error(ret);
>>>                         parms->session_status = -ret; <----
>>>         parms->session_status = tlshd_initialize_ktls(session);
>>>
>>> It also seems mixed, no?
>>
>> The -ETIMEDOUT arm sets session_status to -ret. ret always contains the
>> value -ETIMEDOUT in that arm, so -ret is ETIMEDOUT.
>>
>> This code comes from commit b010190cfed2 ("tlshd: Pass ETIMEDOUT from
>> gnutls to kernel"). Passing -ETIMEDOUT directly to
>> tlshd_log_gnutls_error() does seem a little bogus.
>>
> 
> Got it. I'm trying to align the QUIC err process with tls13 code.
> 
> - tlshd_tls13_client_x509_handshake()
> - tlshd_tls13_client_psk_handshake()
> 
> - tlshd_tls13_server_x509_handshake()
> - tlshd_tls13_server_psk_handshake()
> 
> It seems to me that: When gnutls_xxx() returns errs in these functions,
> it only logs them, and doesn't set any value to parms->session_status,
> but sets it until tlshd_start_tls_handshake().
> 
> So my suggestion is: not touch these functions for QUIC code:
> 
> - tlshd_quic_client_set_x509_session()
> - tlshd_quic_client_set_psk_session()
> 
> - tlshd_quic_server_set_x509_session()
> - tlshd_quic_server_set_psk_session()
> 
> but change their callers tlshd_quic_client/serverhello_handshake():
> 
> @@ -601,14 +599,10 @@ void tlshd_quic_serverhello_handshake(struct
> tlshd_handshake_parms *parms)
>                 ret = -EINVAL;
>                 tlshd_log_debug("Unrecognized auth mode (%d)",
> parms->auth_mode);
>         }
> -       if (ret) {
> -               conn->errcode = -ret;
> -               goto out;
> +       if (!ret) {

   if (ret == GNUTLS_E_SUCCESSS) {

is preferred.


> +               tlshd_quic_start_handshake(conn);
> +               parms->session_status = conn->errcode;
>         }
> -
> -       tlshd_quic_start_handshake(conn);
> -out:
> -       parms->session_status = conn->errcode;
>         tlshd_quic_conn_destroy(conn);

We usually express the return code checking as

   do something
   if (error) {
      error flow
   }

That's why the goto is in there. But I'll leave it up to you in
the quic code.


> and then in tlshd_quic_start_handshake(), keep the code setting
> conn->errcode = errno, but set conn->errcode = EACCES for all
> GNUTLS_E_ places.
> 
> What do you think?

I think that still mixes the error return values for the internal
functions?

The reason I noticed this is I'm trying to add proper Doxygen
comments in here. The internal functions need to be consistent about
returning either only GNUTLS_E or errno. The documenting comments
can't say "returns GNUTLS_E, negative errno, or zero", for example.


-- 
Chuck Lever