505880 - nss_ZRealloc could use a local NSSArena *arena variable to make coverity happy

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement)

Description

[timeless]

coverity tries to watch out for lock abuse, and it can indeed find cases where locks are abused (even i our codebase).

sometimes it's helpful to give coverity a helping hand....

Comment 1

[timeless]

Attachment: could we do this

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Hoes does this help?
What problem does it solve?

Comment 3

[timeless]

Attachment: could we do this

sorry, i missed the key bit!

the analysis looks at the pointer path to the lock

As far as it can tell, the locks i'm quoting here with minuses are distinct
-    PR_Lock(h->arena->lock);
+    PR_Lock(arena->lock);

-    PR_Unlock(new_h->arena->lock);
+    PR_Unlock(arena->lock);

but it should agree that the locks we would be using are the same

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 390123 [details] [diff] [review]
could we do this

Timeless, thank for explaining the issue. 
I am agreed, in principle, to allowing a change like this to 
be made to this function.  It may even make this code easier
to understand by NSS developers.  

However, I am going to ask you to submit one more version of 
this patch which contains a stylistic change.  

Given a pointer variable declared as:

   someTime * varPtr;

long ago, the NSS team had a member who could not bring 
himself to ever code 
   if (!varPtr) 
or 
   if (varPtr)
Instead, he insisted on coding
   if ((someType *)NULL == varPtr) 
or 
   if ((someType *)NULL != varPtr) 

The NSS team leaders are agreed that the form with explicit 
casts and comparisons with NULL is MUCH more difficult to read 
than the simpler form above, and so we are trying to slowly 
rid NSS of code in that undesired style.  So, we ask that, 
whenever a line of code bearing that style is changed for ANY 
reason, that it be changed to the simpler style.  

I'm NOT asking you to do a wholesale conversion of this file 
to the new style.  Please DON'T do that.  I'm merely asking 
you to take the lines that you are already planning to change, 
which bear that style and change them by changing
    (someType *)NULL == 
to just 
    !
and changing 
     (sometType *)NULL !=
to 
   (empty)

When that is done, I think this patch will be ready for r+.
I could do that, but then I'd have to find yet another reviewer.
If you code it, then I can review it an commit it.
Thanks.

Comment 5

[timeless]

Attachment: new nss style

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

1131 new_h = (struct pointer_header *)p; 
1132 new_h->arena = h->arena; 

1140 h->arena = (NSSArena *)NULL; 

1142 PR_Unlock(new_h->arena->lock); 

So, you're saying that it cannot tell that the value of 
new_h->arena->lock at line 1142 is the same as the value of 
h->arena->lock     at line 1131?  That seems lame.  

I can imagine this being an issue in many MANY places in NSS, and really
wouldn't want to change them all just to quiet coverity.  I want to withhold
approval of this checkin until we know just how many such changes to NSS 
are desired to quiet coverity.  

This patch seems correct and satisfies my style needs. :) 
I'd give it r+, but since NSS doesn't have separate review and approval flags
I'm going to withhold the review flag until we know how many more changes 
will be needed to satisfy/quiet coverity.  If it's too many, I'll just say
no to the whole lot of them.

Comment 7

[timeless]

i think this is the only place where it had issues. at least i'm pretty sure i exhausted the set of LOCK complaints.

note that i can't commit to nss, so r+ or no r+ it hardly matters :)

and yeah, personally i felt this warning was lame. otoh, doing it would also help the compiler save on indirections, so it should actually be a general win, even if the original complaint was from coverity.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 390198 [details] [diff] [review]
new nss style

r=nelson. 
I should commit this soon. 
Ping me if I don't.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Checking in nss/lib/base/arena.c new revision: 1.13; previous revision: 1.12