245950 - SSL_RevealURL uses PL_strdup but samples match it with PR_Free

Status: NEW

(NSS :: Libraries, defect, P3)

Description

[Julien Pierre]

I discovered this while reviewing patch 149427 .

SSL_RevealURL uses PL_strdup to create the hostname string that it returns.
According to NSPR headers, the correct function to free that is PL_strfree, not
PR_Free .

A closer look at the source shows that PL_strdup calls the C runtime malloc() .
The matching PL_strfree is defined to call the C runtime free() directly.

PR_Free however may not always call free(), if the NSPR zone allocator is
enabled, but may call pr_zoneFree and get into the following code :

    if (mb->s.magic != ZONE_MAGIC) {
        /* maybe this came from ordinary malloc */
#ifdef DEBUG
        fprintf(stderr,
            "Warning: freeing memory block %p from ordinary malloc\n", ptr);
#endif
        free(ptr);
        return;
    }


Best case, the above code will call free(), and all will be well in release
cases, but there is still a dormant error and an error message in debug cases.

However, if the hostname string is very small, the following will crash due to
going out of bounds :

if (mb->s.magic != ZONE_MAGIC) {

Or example code, and presumably all existing NSS applications that don't just
leak the hostname string, use PR_Free to free it. We can't change this for
binary compatibility reasons.

I suggest that the fix is to change SSL_RevealURL to return a string with memory
allocated by PR_Malloc . This will ensure that existing NSS applications will
work even if the NSPR zone allocator is enabled.

Unfortunately, we probably have to implement yet another version of the strdup
function :-( (there is no PR_strdup).

On the plus side, this is the only occurrence of PL_strdup being called in nss/lib .

Comment 1

[Wan-Teh Chang]

Julien, this is a good suggestion.  In recent versions
of NSS, PORT_Strdup is implemented as a function that
allocates memory using PORT_Alloc.  I think we should
use PORT_Strdup in SSL_RevealURL, document that the
return value of SSL_RevealURL should be freed with
PORT_Free, and optionally allow the (existing sloppy)
use of PR_Free in place of PORT_Free.

Comment 2

[Julien Pierre]

Calling PORT_Strdup in SSL_RevealURL however would tie the PORT_Strdup
implementation to forever using PR_Malloc . If we ever want to change allocator
for the PORT_ functions, we will break the SSL_RevealURL users . Is this really
what we want to do ?

I was thinking of using a static function calling PR_Malloc in sslreveal.c instead .

Comment 3

[Wan-Teh Chang]

PORT_Strdup uses PORT_Alloc, not PR_Malloc.  Note that
in my proposal, allowing the (existing sloppy) use of
PR_Free in place of PORT_Free is optional.  Ideally we
would modify all existing code to use PORT_Free to free
the return value of SSL_RevealURL.

Comment 4

[Julien Pierre]

It isn't possible for us to modify all the existing sloppy application code to
change from PR_Free to PORT_Free here in this case. We have a requirement for
old binaries to keep running with the new libraries. The apps aren't always
delivered together with the libs. They sometimes get patched separately.

Given this requirement, if we switch to PORT_Strdup, it enforces that
PORT_Strdup must call a function that calls PR_Malloc, therefore PORT_Alloc must
always call PR_Malloc and we wouldn't be able to change the allocator.

I believe all existing apps use PR_Free for this case, so it is better not to
change this and accept that as the correct semantics, rather than change it in a
3.x release. We should change it in a major release.