Memory leak in Curl_override_sspi_http_realm

[silveja1]

I've fixed a 1 byte memory leak with Curl_override_sspi_http_realm in curl_sasl_sspi.c on line 297. Without being intimately involved in the project I don't want to check it in.

Here is the fix:

CURLcode Curl_override_sspi_http_realm(const char *chlg,
                                       SEC_WINNT_AUTH_IDENTITY *identity)
{
  xcharp_u domain, dup_domain;

  /* If domain is blank or unset, check challenge message for realm */
  if(!identity->Domain || !identity->DomainLength) {
    // --------------------- ADDED BELOW -----------------------------
    if( identity->Domain )
    {
      // free the domain due to strdup
      free(identity->Domain);
      identity->Domain = NULL;
    }

[jay]

Ah. The default is a dup of a zero length string so it looks like that is possible. I think only if we are updating the domain pointer would we free the old one, so:

--- a/lib/curl_sasl_sspi.c
+++ b/lib/curl_sasl_sspi.c
@@ -316,6 +316,7 @@ CURLcode Curl_override_sspi_http_realm(const char *chlg,
             Curl_unicodefree(domain.tchar_ptr);
             return CURLE_OUT_OF_MEMORY;
           }
+          free(identity->Domain);
           identity->Domain = dup_domain.tbyte_ptr;
           identity->DomainLength = curlx_uztoul(_tcslen(dup_domain.tchar_ptr));
           dup_domain.tchar_ptr = NULL;

Can you try that?

[jay]

Ah. The default is a dup of a zero length string so it looks like that is possible. I think only if we are updating the domain pointer would we free the old one, so:

--- a/lib/curl_sasl_sspi.c
+++ b/lib/curl_sasl_sspi.c
@@ -316,6 +316,7 @@ CURLcode Curl_override_sspi_http_realm(const char *chlg,
             Curl_unicodefree(domain.tchar_ptr);
             return CURLE_OUT_OF_MEMORY;
           }
+          free(identity->Domain);
           identity->Domain = dup_domain.tbyte_ptr;
           identity->DomainLength = curlx_uztoul(_tcslen(dup_domain.tchar_ptr));
           dup_domain.tchar_ptr = NULL;

Can you try that?

[silveja1]

Yes, that works fine. Wouldn't it be better not initialize with strdup("") and just leave as NULL?

Thanks,
Jay

[silveja1]

Yes, that works fine. Wouldn't it be better not initialize with strdup("") and just leave as NULL?

Thanks,
Jay

[jay]

No, we need a pointer to a domain even if it's zero length. There's no guarantee the domain will be replaced, that's why I changed it to free only if there's a replacement. Landed in 742deff, thanks!

[jay]

No, we need a pointer to a domain even if it's zero length. There's no guarantee the domain will be replaced, that's why I changed it to free only if there's a replacement. Landed in 742deff, thanks!