Last Comment Bug 725180 - Create test suite for libmar
: Create test suite for libmar
Status: RESOLVED FIXED
[qa-]
:
Product: Toolkit
Classification: Components
Component: Application Update (show other bugs)
: unspecified
: x86_64 Windows 7
: -- normal (vote)
: mozilla12
Assigned To: Brian R. Bondy [:bbondy]
:
:
Mentors:
Depends on: 699700 708688 708690 708697 728935
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-07 17:36 PST by Brian R. Bondy [:bbondy]
Modified: 2012-03-29 14:58 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
fixed
fixed


Attachments
WIP patch (13.16 KB, patch)
2012-02-07 17:44 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v1. (30.78 KB, patch)
2012-02-19 22:33 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v2. (30.04 KB, patch)
2012-02-20 06:00 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v3 (31.43 KB, patch)
2012-02-21 11:23 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch v4. (38.48 KB, patch)
2012-02-21 20:23 PST, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Libmar test suite. Patch v5. (35.14 KB, patch)
2012-02-23 13:37 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Splinter Review
Misc libmar fixes found from tests. Patch v1. (3.43 KB, patch)
2012-02-23 13:38 PST, Brian R. Bondy [:bbondy]
netzen: review+
Details | Diff | Splinter Review
Fix error handling Patch v1. (3.16 KB, patch)
2012-02-23 17:48 PST, Brian R. Bondy [:bbondy]
robert.strong.bugs: review+
Details | Diff | Splinter Review

Description Brian R. Bondy [:bbondy] 2012-02-07 17:36:08 PST
This bug is to create a test suite for modules/libmar.

This test suite should cover:
- Creating MARs
- Extracting MARs
- Signing MARs
- Verifying signed MARs
- Stripping MAR signatures
- Refreshing Product Info Blocks
Comment 1 Brian R. Bondy [:bbondy] 2012-02-07 17:44:55 PST
Created attachment 595265 [details] [diff] [review]
WIP patch

Work in progress patch.
- Created test suite build files
- Added 4 reference MARs and code for creating 4 MARs from source files, and comparing to verify they are what they should be.
Comment 2 Brian R. Bondy [:bbondy] 2012-02-19 22:33:23 PST
Created attachment 598773 [details] [diff] [review]
Patch v1.

- Tests for Creating MARs
- Tests for Extraction MARs
- Tests for Signing, Verifying, and stripping signatures for MARs, and extracting from signed MARs.
Comment 3 Brian R. Bondy [:bbondy] 2012-02-20 06:00:48 PST
Created attachment 598841 [details] [diff] [review]
Patch v2.

Updated last Windows only patch to work on all platforms (except android).
Comment 4 Brian R. Bondy [:bbondy] 2012-02-21 11:23:15 PST
Created attachment 599275 [details] [diff] [review]
Patch v3

Fixed some build problems and a test problem on Linux and OSX.

In particular for test changes, the verify signmar command line differs on Windows vs other platforms since verification is done via DER file vs NSS config directory.  

In particular for build changes, had to add a prefix.

Also I expanded the MAR product, channel, version checks to be only enabled when the define is enabled in confvars.sh like verification of signed mars.  So windows only for now.

Verified tests are passing now on both OS X and Windows.
Comment 5 Brian R. Bondy [:bbondy] 2012-02-21 20:23:59 PST
Created attachment 599461 [details] [diff] [review]
Patch v4.

Added new tests for:
- For a manually crafted MAR where I manually adjusted the version number of the MAR with a hex editor.  Asserts that signature verification should fail.
- For extracting old no product info block MARs
- For extracting old no product info block MARs that were signed (in use today)
- For verifying old product info block MARs that were signed
Comment 6 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-23 12:41:43 PST
Comment on attachment 599461 [details] [diff] [review]
Patch v4.

>diff --git a/modules/libmar/Makefile.in b/modules/libmar/Makefile.in
>--- a/modules/libmar/Makefile.in
>+++ b/modules/libmar/Makefile.in
>@@ -38,11 +38,16 @@
> 
> DEPTH		= ../..
> topsrcdir	= @top_srcdir@
> srcdir		= @srcdir@
> VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
>-DIRS		= sign verify src tool
>+DIRS = sign verify src tool
>+MODULE = libmar_test
>+
>+ifdef ENABLE_TESTS
>+DIRS += test
nit: I think the current preferred dir name is tests.

>+endif
> 
> include $(topsrcdir)/config/rules.mk
>diff --git a/modules/libmar/TEST/Makefile.in b/modules/libmar/TEST/Makefile.in
Why uppercase?
Comment 7 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-23 12:46:43 PST
Comment on attachment 599461 [details] [diff] [review]
Patch v4.

>diff --git a/modules/libmar/TEST/unit/head_libmar.js.in b/modules/libmar/TEST/unit/head_libmar.js.in
>new file mode 100644
>--- /dev/null
>+++ b/modules/libmar/TEST/unit/head_libmar.js.in
>@@ -0,0 +1,136 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+const BIN_SUFFIX = "@BIN_SUFFIX@";
>+
>+/**
>+ * Compares the binary data of 2 files.
Just "Compares binary data of two parameters and throws if they aren't the same." since the files aren't involved at this point

Might as well add arr1 and arr2

>+ * Throws on mismatch, does nothing on match.
>+*/
>+function compareBinaryData(arr1, arr2) {
>+  do_check_eq(arr1.length, arr2.length);
>+    for (let i = 0; i < arr1.length; i++) {
>+      if (arr1[i] != arr2[i]) {
>+        throw "Data differs at index " + i;
indentation is off on the above 3 lines

>+    }
>+  }
>+}
Comment 8 Brian R. Bondy [:bbondy] 2012-02-23 12:49:45 PST
> include $(topsrcdir)/config/rules.mk
>diff --git a/modules/libmar/TEST/Makefile.in b/modules/libmar/TEST/Makefile.in
> Why uppercase?

I can't explain why it went uppercase, my local patch that has since been qrefresh'ed no longer has it uppercase.
Comment 9 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-23 12:53:07 PST
Comment on attachment 599461 [details] [diff] [review]
Patch v4.

>diff --git a/modules/libmar/TEST/unit/head_libmar.js.in b/modules/libmar/TEST/unit/head_libmar.js.in
>new file mode 100644
>--- /dev/null
>+++ b/modules/libmar/TEST/unit/head_libmar.js.in
>@@ -0,0 +1,136 @@
>...
>+/** 
>+ * Reads a file's data and returns it
>+ *
>+ * @param file The file to read the data from
>+ * @return a byte array for the data in the file.
>+*/
>+function getBinaryFileData(file) {
>+  let fileStream = Components.classes[
nit I'd go with
const Cc = Components.classes;
const Ci = Components.interfaces;
etc. to lessen the additional lines to stay under 80
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-23 13:00:15 PST
Comment on attachment 599461 [details] [diff] [review]
Patch v4.

Could you split out the c++ code changes and their makefiles into a separate patch?
>diff --git a/modules/libmar/tool/Makefile.in b/modules/libmar/tool/Makefile.in
>--- a/modules/libmar/tool/Makefile.in
>+++ b/modules/libmar/tool/Makefile.in
>@@ -49,16 +49,20 @@ USE_STATIC_LIBS = 1
> endif
> 
> # The mar executable is output into dist/host/bin since it is something that
> # would only be used by our build system and should not itself be included in a
> # Mozilla distribution.
> HOST_PROGRAM = mar$(HOST_BIN_SUFFIX)
> PROGRAM = signmar$(BIN_SUFFIX)
> 
>+# Don't link the updater against libmozglue. See bug 687139
Copy and paste error. This isn't the updater

>+MOZ_GLUE_LDFLAGS =
>+MOZ_GLUE_PROGRAM_LDFLAGS =
>+

r=me for the c++ code changes and their makefiles. I'm still reviewing the tests but if you could please resubmit with the comments addressed so far.
Comment 11 Brian R. Bondy [:bbondy] 2012-02-23 13:37:09 PST
Created attachment 600167 [details] [diff] [review]
Libmar test suite.  Patch v5.

Updated review comments and removed C++ parts and related Makefiles.
Comment 12 Brian R. Bondy [:bbondy] 2012-02-23 13:38:33 PST
Created attachment 600168 [details] [diff] [review]
Misc libmar fixes found from tests. Patch v1.

Marking the C++ parts and their Makefile changes as r+ as per comment 10:

rs:
> r=me for the c++ code changes and their makefiles.
Comment 13 Brian R. Bondy [:bbondy] 2012-02-23 17:48:20 PST
Created attachment 600265 [details] [diff] [review]
Fix error handling Patch v1.

When testing the error handling for cert verification errors, I found that we will try to re-use the service for the new service specific security error codes after re-downloading the full MAR.  So this error handling should follow the same as the other service specific error codes (fallback to pending before trying to re-download the MAR).
Comment 14 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-24 13:00:52 PST
Comment on attachment 600167 [details] [diff] [review]
Libmar test suite.  Patch v5.

>diff --git a/modules/libmar/tests/unit/test_sign_verify.js b/modules/libmar/tests/unit/test_sign_verify.js
>new file mode 100644
>--- /dev/null
>+++ b/modules/libmar/tests/unit/test_sign_verify.js
>@@ -0,0 +1,187 @@
>...
>+  function verifyMAR(signedMAR, wantSuccess) {
>+    if (wantSuccess === undefined) {
>+      wantSuccess = true;
>+    }
>+    // Get a process to the signmar binary from the dist/bin directory.
>+    let process = Cc["@mozilla.org/process/util;1"].
>+                  createInstance(Ci.nsIProcess);
>+    let signmarBin = do_get_file("signmar" + BIN_SUFFIX);
>+
>+    // Make sure the signmar binary exists and is an executable.
>+    do_check_true(signmarBin.exists());
>+    do_check_true(signmarBin.isExecutable());
>+
>+    let DERFile = do_get_file("data/mycert.der");
>+
>+    // Will reference the arguments to use for verification in signmar
>+    let args;
>+
>+    // The XPCShell test wiki indicates this is the preferred way for 
>+    // Windows detection.
>+    var isWindows = ("@mozilla.org/windows-registry-key;1" 
>+                     in Cc);
nit: why 2 lines?

I'm a little concerned that there will be times where Windows file in use will prevent the directory removals but it would be better to fix those if they come up.

Looks good!
Comment 15 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-24 13:08:04 PST
Comment on attachment 600265 [details] [diff] [review]
Fix error handling Patch v1.

This will need to be backed out at the same time that these checks are made when not using the service.
Comment 16 Brian R. Bondy [:bbondy] 2012-02-24 13:33:59 PST
Forgot the 2 line -> 1 line nit before landing but I'll fix this once I do the patch for doing the security checks non service, made a local note.
Comment 17 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-24 13:35:44 PST
Just a nit... not a problem :)
Comment 19 Brian R. Bondy [:bbondy] 2012-02-24 13:43:01 PST
and...
http://hg.mozilla.org/mozilla-central/rev/2b1a53905350
Comment 20 Brian R. Bondy [:bbondy] 2012-02-27 05:46:20 PST
> I'm a little concerned that there will be times where Windows file in use 
> will prevent the directory removals but it would be better to fix those if 
> they come up.

By the way I'm not sure why some updater tests are like that but it's something that we shouldn't need to worry about. I think there is something else going on  there (like an unclosed handle somewhere) but I'll look into that at a future date if I have more time :)
Comment 21 Robert Strong [:rstrong] (use needinfo to contact me) 2012-02-27 11:23:13 PST
I wouldn't worry about it unless they come up. Also, we've seen cases where we'll run into file in use on opt but not debug with update and other tests.
Comment 22 Brian R. Bondy [:bbondy] 2012-02-29 18:24:45 PST
http://hg.mozilla.org/releases/mozilla-aurora/rev/18840d884c97

Note You need to log in before you can comment on or make changes to this bug.