Memory leak tests don't detect same ordinary failures as all.sh

RESOLVED FIXED in 3.11.9

Status

NSS
Test
P2
normal
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Slavomir Katuscak)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

6.23 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Julien Pierre
: superreview+
Details | Diff | Splinter Review
In bug 400841 comment 5, Slavo wrote:

> Memory leak checking doesn't check return values of NSS tools and, 
> if strsclnt fails it's not detected. Test is marked as FAILED only when 
> there is new leak found. 

He went on to say that if the same failures always occurred in both leak 
test runs and non-leak test runs, that would be OK, but now we're having 
test program failures that occur only in leak-test runs, and these failures
are going undetected.

So this bug calls the for the leak tests to detect all the same types of 
failure that the non-leak tests detect.  If a failure would turn a non-leak
test run orange (in tinderbox) then a leak test run should also turn orange
when it encounters that failure.
(Reporter)

Updated

10 years ago
Priority: -- → P2
(Assignee)

Updated

10 years ago
Target Milestone: 3.11.8 → 3.11.9
(Assignee)

Comment 1

10 years ago
Created attachment 292590 [details] [diff] [review]
Patch.

This patch adds 3 things:

1. Functions run_commands_dbx and run_commands_valgrind returns return value which returns tested command.
2. Added checking of return values, failures are reported to both results.html and output.log (only failures).
3. Fixed issue from bug 400841 comment 2. I compared memory leak tests with basic ssl tests and there is always used nickname as parameter in strsclnt (-n TestUser). This was not used in memory leak tests before - it caused failures when client DB was in FIPS mode, otherwise it worked. Adding this parameter fixes the issue, please let me know if this is correct (FIPS feature) or it's a bug (should work also without parameter).
Attachment #292590 - Flags: review?(nelson)
(Reporter)

Comment 2

10 years ago
Comment on attachment 292590 [details] [diff] [review]
Patch.

I have one concern about this patch, in the following hunk.

>- echo "${DBX_CMD}" | ${DBX} ${COMMAND} 2>/dev/null | grep -v Reading 1>&2
>+	
>+ output=`echo "${DBX_CMD}" | ${DBX} ${COMMAND} 2>/dev/null | grep -v Reading`
>+ echo "$output" 1>&2
>+	
>+ echo "$output" | grep "exit code is 0"
>+ return $?

This puts the entire output of the dbx command into a shell variable 
named "output".  There's some limit to the size of the content of a 
shell variable, but I don't know what that limit it.  Is there any 
chance that the output of the dbx command will exceed that limit?
(Assignee)

Comment 3

10 years ago
I checked bash manpage and I didn't find any limit. 
Then I tested it on my laptop with 2GB RAM, with command q=`cat /dev/urandom` it was allocating memory until 2 gigs and than bash hanged (needed to use kill -9).

I think if we exceed limit like this, it means that we have much bigger problem with leaks than with limitation of bash.  
(Reporter)

Comment 4

10 years ago
I tested the limit on another POSIX shell, and found it was less than 32K.
Unless you have reason to believe that the output of DBX will never exceed
32K, I think this patch will have to use something other than a shell 
variable to hold the dbx output.
(Assignee)

Comment 5

10 years ago
Created attachment 295929 [details] [diff] [review]
Patch v2.

Using temporary file instead for variable for output of dbx.
I don't think this is really needed, but 32K is a limit which we can exceed in some cases.
Attachment #292590 - Attachment is obsolete: true
Attachment #295929 - Flags: review?(nelson)
Attachment #292590 - Flags: review?(nelson)
(Reporter)

Comment 6

10 years ago
Comment on attachment 295929 [details] [diff] [review]
Patch v2.

Please make one change, then r=nelson

>+	cat ${TMP_DBX} | grep "exit code is 0"

Change that line to:

        grep "exit code is 0" ${TMP_DBX} 

I've asked Julien for second review for branch.
Attachment #295929 - Flags: superreview?(julien.pierre.boogz)
(Reporter)

Updated

10 years ago
Attachment #295929 - Flags: review?(nelson) → review+

Updated

10 years ago
Attachment #295929 - Flags: superreview?(julien.pierre.boogz) → superreview+
(Assignee)

Comment 7

10 years ago
Trunk:
Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.24; previous revision: 1.23
done
(Assignee)

Comment 8

10 years ago
Branch:
Checking in memleak.sh;
/cvsroot/mozilla/security/nss/tests/memleak/memleak.sh,v  <--  memleak.sh
new revision: 1.1.2.19; previous revision: 1.1.2.18
done
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.