400439 - reduce redundancy in file of ignored leak stacks

Status: RESOLVED FIXED

(NSS :: Test, enhancement, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

The "ignored" file of known leak stacks has much unhelpful redundancy.
Ideally it would contain one line per conceptual leak.  
When that one leak is fixed, that one line would be removed.  
But today it contains many lines for a single conceptual leak.  
I want to eliminate the redundancy and reduce the file size, 
approaching the lower bound of one line per conceptual leak.

There are (at least) two reasons for this redundancy.

1) compound objects generate multiple stacks when leaked.
Consider an object like a PRLock or a PKIX object, which contain
several parts, each of which is separately allocated.  When a single
PRLock or single PKIX object is leaked, all the indivudual parts of 
the object are leaked, each with a separate stack.  But the leak is
really of a single compound object, and when it is fixed, all the 
stacks for all the parts of it will go away.  

So, I changed the stacks for compound objects.  For example, I 
changed all the stacks that leak PRLocks, so that PR_NewLock/** 
is the last thing that appears on the line, then eliminated duplicate
lines.  That eliminated MANY redundant lines.

2) Some functions always leak, no matter how they are called, nor in 
which program they are called.  If they are called in many paths, in 
many programs, they generate many leak stacks.  But when the leaky 
function is fixed, all the stacks that call it will go away.  

I identified numerous leaky functions, such as cert_VerifyCertChainPkix
that appear in many different call stacks, and changed all those stacks
to begin with **/cert_VerifyCertChainPkix  then eliminated duplicate 
lines from the file.  This removed many redundant lines.

The result of those changes is the patch attached here.  

After I made all these changes, I noticed one more thing that I want to 
ask about.  

There are many stacks that appear twice, whose only difference is the 
presence of the _Util suffix in one stack, e.g. these two

ocspclnt/main/CERT_SetOCSPDefaultResponder/PORT_Strdup/**
ocspclnt/main/CERT_SetOCSPDefaultResponder/PORT_Strdup_Util/**

Do both of these stacks occur on the trunk?
Or does this one file contain both trunk stacks and branch stacks?
I don't want branch-only stacks on the trunk's ignored file, nor trunk-only
stacks in the branch's ignored file.

I invite comments from you all.

Slavo, please review and ensure that lines with "**" in the middle will work,
e.g.  strsclnt/**/thread_wrapper/do_connects/PR_Send (...)

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch to file of ignored leak stacks

Comment 2

[Slavomir Katuscak]

> There are many stacks that appear twice, whose only difference is the 
> presence of the _Util suffix in one stack, e.g. these two
> 
> ocspclnt/main/CERT_SetOCSPDefaultResponder/PORT_Strdup/**
> ocspclnt/main/CERT_SetOCSPDefaultResponder/PORT_Strdup_Util/**
> 
> Do both of these stacks occur on the trunk?

Yes, both stacks are from trunk. First one was there before libutil was integrated, second after. I didn't check if old stacks are still there or not (it can be problem, because some of them are platform specific and some of them doesn't appear always). I only added new stacks and let all stacks there.

> Or does this one file contain both trunk stacks and branch stacks?

No, I manage branch list separately.

> I don't want branch-only stacks on the trunk's ignored file, nor trunk-only
> stacks in the branch's ignored file.

Agree.

Comment 3

[Slavomir Katuscak]

Nelson, I have some suggestions how to improve your patch - many bugs can be simplified to one pattern - it's enough to detect them, but in some cases there is a risk that using shorter patterns will cause that we don't detect new bugs which matches some ignored pattern.

Please check my suggestion and select the ones that you think are OK:

Bug 169313:
**/SECMOD_LoadModule/SECMOD_LoadPKCS11Module/secmod_ModuleInit/builtinsC_Initialize/NSSCKFWC_Initialize/nssCKFWInstance_Create/NSSArena_Create/nssArena_Create/arena_add_pointer/nssPointerTracker_initialize/call_once/**

Bug 367374:
After matching other stacks last line is not needed, you can remove it:
ocspclnt/main/PR_GetSpecialFD/**

Bug 370536:
First 2 stacks can be simpified to:
**/nssArena_Destroy/arena_remove_pointer/nssPointerTracker_remove/PL_HashTableRemove/PL_HashTableRawRemove/DefaultAllocTable/**

Bug 393174:
Can be removed completely, all stacks are already commented and we don't see them.

Bug 393181:
ocspclnt/main/CERT_SetOCSPDefaultResponder/**

Bug 397478:
selfserv/main/SSL_ConfigServerSessionIDCache/** 

Bug 397483:
**/PR_CallOnce/InitializeArenas/PR_NewLock/**

Bug 399296:
**/PKIX_PL_Cert_IsCertTrusted/pkix_pl_Pk11CertStore_CheckTrust/CERT_GetCertTrust/CERT_LockCertTrust/**/PR_NewLock/**

Bug 399300:
**/pkix_Throw/PKIX_Error_Create/PKIX_PL_Object_Alloc/**

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Slavo, in your comment 3, you list 9 bugs and make suggestions for 3 of them.
What are you proposing for the other 6?

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 285492 [details] [diff] [review]
patch to file of ignored leak stacks

Alexei, please review

Comment 6

[Alexei Volkov]

Comment on attachment 285492 [details] [diff] [review]
patch to file of ignored leak stacks

It is an improvement. Couple things can be changed:

1) Remove stacks related to bug 391774. The bug is fixed.

2) Add **/PKIX_PL_Object_Alloc/** instead of all stacks that have this function call. The reason: we should not leak any objects eventually, but we currently have the bug 397832 that make libpkix leak objects in case of errors. For now we can ignore, while we fixing that bug.

Comment 7

[Slavomir Katuscak]

Nelson, in other 6 I didn't expected big changes and your patch looks OK, but for completeness:

Bug 367376:
**/_PR_CreateThread/pthread_create@@GLIBC_2.1/**

Bug 367384: 
**/PR_LoadLibraryWithFlags/**
**/pr_LoadLibraryByPathname/**

Bug 393181:
ocspclnt/main/PL_CreateOptState**
When using current parser this should match both stacks.

Bug 397486:
**/ssl_Send/ssl_SecureSend/ssl_Do1stHandshake/ssl2_BeginClientHandshake/ssl_LookupSID/**

Bug 397487 & bug 398267:
**/__rpc_getconfip/setnetconfig/**

Bug 391774:
**/PKIX_Initialize/PKIX_PL_Initialize/PR_NewLock/**

Comment 8

[Slavomir Katuscak]

Good to see in bugzilla comments that we are matching also bugs which are already fixed.

Bugs 393174 and 391774 can be removed. Bug 397487 is closed as WONTFIX (libc problem), so we should keep it there.

Comment 9

[Slavomir Katuscak]

Nelson, please send me feedback to my comments. Thanks.

Comment 10

[Nelson Bolyard (seldom reads bugmail)]

Slavo, Bring my patch up to date on the trunk and ask me to review it.

Comment 11

[Slavomir Katuscak]

Attachment: Patch.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288984 [details] [diff] [review]
Patch.


> #367374
>-*/main/PR_Init/_PR_ImplicitInitialization/**
>-*/main/PR_Init/_PR_InitCMon/ExpandMonitorCache/PR_Calloc/calloc
>-*/main/PR_Init/_PR_InitCMon/ExpandMonitorCache/PR_NewMonitor/PR_Calloc/calloc
>-*/main/PR_Init/_PR_InitCMon/PR_NewLock/PR_Calloc/calloc
>-*/main/PR_Init/_PR_InitLinker/PR_Calloc/calloc

Slavo, have you actually tested this patch?  
Does the tinderbox remain green with it in place?

Some of the old leak stacks do not have matching replacements in the patch.
For example, the last line quoted above has no replacement.
So, either that leak has been fixed, or else it will turn the tree orange
if it occurs with this patch in place.

Comment 13

[Slavomir Katuscak]

Hi Nelson,

I checked my patch today, it works with one small correction (** at the beginning of stack for bug 397486).

Stacks from bug 367374 you mentioned are matched, /main/ is replaced by ** in patterns in ignored file.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 288984 [details] [diff] [review]
Patch.

Please attach a patch showing exactly the changes that were committed.

Comment 15

[Slavomir Katuscak]

Attachment: Patch v2.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 291179 [details] [diff] [review]
Patch v2.

r=nelson

Comment 17

[Slavomir Katuscak]

Checking in ignored;
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.55; previous revision: 1.54
done