[squid-dev] [PATCH] GnuTLS session redo

[Alex Rousskov]

On 08/03/2016 11:57 PM, Amos Jeffries wrote:
> +Security::SetSessionResumeData(const Security::SessionPointer &s, const Security::SessionStatePointer &data)
> +{
> +    if (data) {
> +#if USE_OPENSSL
> +        (void)SSL_set_session(s.get(), data.get());
> +#elif USE_GNUTLS
> +        (void)gnutls_session_set_data(s.get(), data->data, data->size);
> +#endif
> +    }
> +}

Why cast to (void) here? The current code does not do that AFAICT.
Casting the result to (void) usually means

* I know this may fail, but I do not care.

It should be accompanied by a comment that explains why we do not care,
unless the code makes it obvious (the patched code does not).


Not casting may mean many things, including:

* It returns void.
* I did not even think about it.
* I know it may fail, but I either do not know how to handle its failure
or just do not have the time to handle it.

AFAICT, in this particular case, the current code probably matches one
of the last two bullets. If I am right, then there are two correct ways
to deal with it IMO:

1. Return a boolean success/failure result.
   The caller will still ignore the result.

2. Remove "(void)". Ideally, add a "// TODO: throw on failures" comment.

My weak recommendation is for #2.


> +Security::SessionIsResumed(const Security::SessionPointer &s)
> +{
> +    return
> +#if USE_OPENSSL
> +        SSL_session_reused(s.get()) == 1;
> +#elif USE_GNUTLS
> +        gnutls_session_is_resumed(s.get()) != 0;
> +#else
> +        false;
> +#endif
> +}
> +
> +void
> +Security::GetSessionResumeData(const Security::SessionPointer &s, Security::SessionStatePointer &data)
> +{
> +    if (!SessionIsResumed(s)) {
> +#if USE_OPENSSL
> +        data.reset(SSL_get1_session(s.get()));
> +#elif USE_GNUTLS
> +        gnutls_datum_t *tmp = nullptr;
> +        (void)gnutls_session_get_data2(s.get(), tmp);
> +        data.reset(tmp);
> +#endif
> +    }
> +}

Can we establish some rules for when to use "#else" in
OpenSSL/GnuTLS/Security contexts? The inconsistency illustrated above is
a red flag. I do not think "use #else if it is needed to compile" is the
rule used for the above code because Security::GetSessionResumeData()
may produce an "unused parameter 'data'" warning, killing the build.


> +/// Retrieve the data needed to resume this session on a later connection
> +void GetSessionResumeData(const Security::SessionPointer &, Security::SessionStatePointer &);

> +void
> +Security::GetSessionResumeData(const Security::SessionPointer &s, Security::SessionStatePointer &data)
> +{
> +    if (!SessionIsResumed(s)) {

The implementation does not seem to match the description. The
implementation matches the current code, but the description says
nothing about !SessionIsResumed() precondition. Please either change
description and rename OR test the precondition in the callers,
whichever you think is better long-term.


Thank you,

Alex.