superreview cancelled: [Bug 115951] freebl dynamic library is never unloaded by libsoftoken or libssl. Also tiny one-time leak in freebl's loader.c : [Attachment 239727] Incorporate Wan-Teh's feedback

Julien Pierre <> has cancelled Julien Pierre
<>'s request for superreview:
Bug 115951: freebl dynamic library is never unloaded by libsoftoken or libssl.
Also tiny one-time leak in freebl's loader.c

Attachment 239727: Incorporate Wan-Teh's feedback

------- Additional Comments from Julien Pierre <>
1) I decided to still add the check for blLib, because the implementation for
PR_UnloadLibrary is in a different module, and may still crash in a different
version of NSPR than the one for which I checked the source, however unlikely
that may be.

2) I added assertions in loader.c that PR_UnloadLibrary succeeded. I hope no
code checking tools will complain about that !

3) re: comment 33, the previous patch didn't include
if (on == false_value ) { do_false_case }
if (on == true_value ) { do_true_case }

But rather :

if (on == true_value ) { do_true_case } else { do_false_case }

This means either do_true_case or do_false_case are always executed.

You object and recommend using instead :

if (on != false_value { do_true_case } else { do _false_case }

In both cases, either the true or false code path is always executed.
If on is a random_value distinct from true_value and false_value, then my test
construct would call do_false case, while yours would call do_true case.

Given that 2 (or any random_value not PR_TRUE or PR_FALSE) is an undefined
value for PRBool, I don't agree that it is preferrable to call do_true_case
over do_false_case for undefined values . I have implemented your suggestion in
the interest of getting the patch out quickly. It is no worse and no better
than the original code.

If you want to be strict, an undefined input value needs to be handled as a
third case, an error case, ie:

if (on == true_value ) { do_true_case } else if (on == false_value) {
do_false_case } else { do_error_case };

I think that would clearly be a better solution than either my original patch
or your suggestion, but it is also a change that should be made for all options
that take a PRBool, ie. all of them except SSL_REQUIRE_CERTIFICATE, I believe.
I only wanted to change the implementation for SSL_BYPASS_PKCS11 in this patch,
so I abstained from making that change.

Lastly, not everyone commits the numeric value of every enum to memory. That's
the compiler's job. I personally find the following to be unreadable code :
if (on)
When on is a PRBool, this actually means :
if ((PRBool) 0 != on)
That is confusing unless you know the definition of PRBool . To make sense of
this expression requires the reader to check prtypes.h to figure out that
PR_FALSE is 0, and that the expression actually means :
if (PR_FALSE != on )

IMO, all code using PRBool should be independent of the numeric values assigned
to PR_FALSE and PR_TRUE. The only assumption that is reasonable to make which
is obvious from the names PR_FALSE and PR_TRUE is that they are distinct
values. I realize a lot of existing NSPR code doesn't comply with this, but for
new code, it is easy to comply by always spelling out the enum names rather
than referring to numeric values such as 0.
9/27/2006 11:07:06 PM 29307 articles. 3 followers. Post Follow

0 Replies

Similar Articles

[PageSpeed] 31
Get it on Google Play
Get it on Apple App Store