398882 - leading "**" in "ignored" file matches all leaks

Status: RESOLVED FIXED

(NSS :: Test, defect, P1)

Description

[Robert Relyea]

recently the ignored file was updated with a new entry:

**/some/stack/**

Our scripts (both the old and the new treat ** as 'match all the rest' rather than match any number of stacks.

Updating our scripts to the new semantic may be a good idea, but until then we should remove this entry in the ignore file (which will match any stack currently).

bob

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Bob, do you mean to say that 
    **/some/stack/**
means the same as 
    **
?? 

If so, that is an inexcusable priority 0 blocker bug.  
It contradicts the definition of "**" that was documented when the feature
was first introduced.  

Christophe, maybe you can help get this fixed ASAP.

Comment 2

[Slavomir Katuscak]

Attachment: Patch v1.

Originally I planned script/stacks to use ** only at the end of the stack, so it's not a bug, it's how it was designed.

This patch adds option to use ** also inside the stack, with limitation there can be only normal string (function name) or end of stack after **. (Combinations like /**/**/ or /**/*/ are not supported.)

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

This whole task can be trivially solved with regular expressions.
You can replace each "**" with ".*" and each single * with "[^/]*"
and grep will do the entire match for you.  Grep won't have any problem
with multiple wildcard patterns in a single expression.
The problem then becomes to construct a stack from the output of 
valgrind or DBX that is in the same format as the ignored stacks,
and let grep determine if there is a match or not.

Comment 4

[Robert Relyea]

Comment on attachment 283990 [details] [diff] [review]
Patch v1.

r+, but see nelson's comments about regular expressions...

Comment 5

[Robert Relyea]

Also, the comment for the variable i should be 
# i - which ignore stack we are testing.
Your patch works with the current format of ignore.

---------------------------------------------

If you change the ignore file to include regular expressions, then you can use a single dimension array without the complicated split function. Your check for the ignore match would be:

    for (i=0; i < count ; i++) {
         if (stack ~ ignore[1]) {
              match_found =1;
              break;
         }
    }

That will simplify the awk script immensely (awk used the built in regular expression engine that grep uses).

Also, you'll want to add ^ and $ at the beginning and end of most of your stacks if you do this.


bob

Comment 6

[Nelson Bolyard (seldom reads bugmail)]

If we switch to regular expressions, then 
    **/some/stack**
could be changed to
    ^.*/some/stack/.*$
or simply to 
    /some/stack/
which is simpler.  

Stacks like 
    some/*/*/*/*/stack
would have to change to
    some/[^/]*/[^/]*/[^/]*/[^/]*/stack
which is uglier, but we could live with it.

If we took the simpler approach described above (which drops ** altogether)
then we could stay with the existing some/*/*/*/*/stack syntax in the 
ignored file, and form regular expressions from the ignored file lines by
substituting '[^/]*' for '*'.  That would be less ugly.

Comment 7

[Christophe Ravel]

> or simply to 
>    /some/stack/

Well, next time somebody is going to complain that stacks like some/**/stack are not matched and we will have to reopen the bug again.

It looks like not everybody is familiar with regular expressions. Some may find it is ugly to have strings like some/[^/]*/[^/]*/[^/]*/[^/]*/stack in the ignore file.

I suggest that we:
1/ use % and %% instead of * and ** because * has a special meaning in regular expressions. I checked that % cannot appear in a C-function name.

2/ we process % (replace with [^/]*) and %% (replace with .*) within the awk script. We can also and ^ at the beginning and $ at the end (since we have to do processing anyway).

This solution keeps the ignore file nice and tidy, and allow to use some special regular expressions when required.

I am working on an implementation of this. Just need to make some verifications.

Comment 8

[Robert Relyea]

If we don't go with regular expressions, then I suggest we keep the existing '*'. Regular expression syntax is basic to most unix operations and well documented in a number of places (you can find tons of tutorials on line).

I don't think changing to % an %% adds any value. You can do the same processing on * and ** as you would with % and %%, you just need to escape the '*' in the awk script. (gsub will do the trick for you).

I'm ok with keeping the existing syntax we currently have.

We should get this fix quickly, I want to make sure there are not regressions in leaks in the tree.

bob

Comment 9

[Christophe Ravel]

Yeah, I tried the % but they look confusing between // (like /%/%/ and /%%/).
The other reason for using % is that you don't mess up everything when you start replacing * and ** with regular expressions that contain other *. But there are some ways around this.

Comment 10

[Robert Relyea]

Attachment: new check_ignored function

I've updated my analyze.sh program... here's the check_ignore function used in it if it can help.

Comment 11

[Christophe Ravel]

I have pretty much the same script. It is taking me some time to verify that it really works.

Comment 12

[Christophe Ravel]

Attachment: check for ignored stacks by converting * and ** into regular expressions

The ignore file format remains unchanged.

During parsing:
* is replaced with [^/]*
** is replaced with .*
^ and $ are added at the beginning and the end of the ignored stack

Comment 13

[Slavomir Katuscak]

Comment on attachment 284073 [details] [diff] [review]
check for ignored stacks by converting * and ** into regular expressions

Good idea! At the time when I was working on this format, I didn't expected this to become so important thing, originally I planned only to copy all found stacks to ignored file, * and ** were added because of many similar stacks and this construction was most simple to manage in ignored file.

Comment 14

[Christophe Ravel]

I don't think there should be any difference for memory leak testing script between the trunk and the branch. Only the list of ignored leaks should be different.
Thus changing the target to 3.11.8.

Comment 15

[Christophe Ravel]

Committed on the trunk:

Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.16; previous revision: 1.15

Will commit to NSS_3_11_BRANCH once the second review is granted.

Comment 16

[Robert Relyea]

Comment on attachment 284073 [details] [diff] [review]
check for ignored stacks by converting * and ** into regular expressions

r- I see insufficient justification for going to % an %%. look at my example for how to parse using * and **.

Comment 17

[Christophe Ravel]

Both solutions are valid and use the same number of gsub.
Your solution duplicates the replacement of * with [^/]* which is a potential source of error.

Comment 18

[Robert Relyea]

Sorry, my mistake....

I initially thought you were replacing * with % in the ignore file.
Temporarily replacing them in the line in fine. (I rejected that solution myself because it means * and % is not indistingishable).

My complaint was I thought you were changing the format of the ignore file.

(BTW, my solution is careful not to duplicate the the * from the .* generated by ** by only matching ^* and /* (alternately we could match [^\\.]\\* as well;).

Anyway your use of % as a placeholder does not deserver an r-...

Comment 19

[Robert Relyea]

Comment on attachment 284073 [details] [diff] [review]
check for ignored stacks by converting * and ** into regular expressions

r+

Comment 20

[Christophe Ravel]

Thanks Bob. Your solution was correct as well.

Committed on NSS_3_11_BRANCH:
Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.1.2.9; previous revision: 1.1.2.8

Comment 21

[Slavomir Katuscak]

Fixed bug in AWK settings (AWK variable was not set) in branch:

Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.1.2.10; previous revision: 1.1.2.9
done