Closed Bug 248751 Opened 20 years ago Closed 20 years ago

signtool should create XPInstall compatible signed archives

Categories

(NSS :: Tools, enhancement, P3)

enhancement

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: tagnarth, Assigned: nelson)

Details

Attachments

(2 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040614 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040614 Firefox/0.8

Currently XPInstall only recognizes .xpi's as signed if the pkcs7 file is first
in the archive. Currently signtool places it dead last in the archive. This
causes a perfectly signed archive to appear as unsigned when an archive is being
installed through XPInstall. 

Adding a new option to signtool that would arrange an archive in an XPInstall
compatible order would be a great benefit to extension developers. I have
created a patch that would add a new option -X. This option when used with -Z
only will create an archive that meets the expectations of XPInstall. If the -X
option is not set archives are created exactly the same as before.

Currently the patch is located at http://www.j-maxx.net/signtool_xpi.patch . I
will add it as an attachment in a followup post. It was created against
NSS_3_9_RTM with the command "cvs diff NSS_3_9_RTM signtool"  

I have successfully compiled NSS with this patch on Windows XP and Linux. Both
compiles returned the same resulting archives.

Reproducible: Always
Steps to Reproduce:
1. Create a signed archive with signtool 
signtool -d "certificate directory" -k "cert nickname" -p "password" -X -Z
"file.xpi" directory-tree
2. Load the file.xpi into Mozilla, FireFox, Thunderbird or NS 7.1 by dragging it
over the application. Thus triggering XPInstall. This can also be accomplished
via a web page. Example of compatible and in compatible signed archives here
http://www.j-maxx.net/signtool.php
This patch file introduces a new option into signtool . The -X option used in
conjunction with -Z will create an archive compatible with what XPInstall
expects for a signed archive. It does error checking to make sure when -X is
passed that -Z is also passed. If -X is not passed the resulting archive and
steps to create the archive are the same as the unpatched signtool.

The method used I know is not the best.(manifest.mf and zigbert.sf are created
twice). I have had 3 ideas on ways to implement the option and this was the
easiest and fastest. The other two would involve modifying multiple functions
instead of just one. I've tested this method and found when the -X option is
selected there is a 9% increase in completion time against a regular archive
creation. I expect that the other methods will also create a similar increase.
I'm willing to write out the other methods if so wished. They may take a bit to
flesh out.
Comment on attachment 151772 [details] [diff] [review]
Patch File to include new -X option for XPI archive creation

Index: signtool/README
===================================================================
RCS file: /cvsroot/mozilla/security/nss/cmd/signtool/README,v
retrieving revision 1.1
diff -r1.1 README
2c2
<			   1.3 Release Notes
---
>                          1.4 Beta Release Notes
9a10,18
> === New Features in 1.4 Beta
> =======================
> One new option (-X) has been added to create a Mozilla aware signed XPI archive. 
> The option must be accompanied by the -Z option. This new option
> creates a JAR file with the META-INF/zigbert.rsa/dsa file as the first file in 
> the archive instead of the default third to last. This will enable the archive
> to be seen as signed by products incorporating XPInstall. i.e. .xpi extensions
> for FireFox or Mozilla.
> 
Index: signtool/sign.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/cmd/signtool/sign.c,v
retrieving revision 1.8
diff -r1.8 sign.c
76,85c76
<	if(zip_file) {
<		zipfile = JzipOpen(zip_file, NULL /*no comment*/);
<	}
< 
<   manifesto (tree, install_script, recurse);
< 
<   if (keyName)
<     {
<     status = create_pk7 (tree, keyName, &keyType);
<     if (status < 0)
---
>     if(xpi_arc)
87,89c78,149
<	PR_fprintf(errorFD, "the tree \"%s\" was NOT SUCCESSFULLY SIGNED\n",
tree);
<		errorCount++;
<	exit (ERRX);
---
>          manifesto (tree, install_script, recurse);
>          if(zip_file) {
>            zipfile = JzipOpen(zip_file, NULL /*no comment*/);
>          }	      
>          if (keyName)
>            {
>             status = create_pk7 (tree, keyName, &keyType);
>             if (status < 0)
>             {
>               PR_fprintf(errorFD, "the tree \"%s\" was NOT SUCCESSFULLY SIGNED\n", tree);
>               errorCount++;
>               exit (ERRX);
>             }
>     	 }
> 
> 	     /* If creating XPI compatible archive add rsa/dsa first 
>           then create manifest lists*/
>           
>          /* rsa/dsa to zip */
>          sprintf (tempfn, "META-INF/%s.%s", base, (keyType==dsaKey ? "dsa" : "rsa"));
>          sprintf (fullfn, "%s/%s", tree, tempfn);
>          JzipAdd(fullfn, tempfn, zipfile, compression_level);
>   
> 	     manifesto (tree, install_script, recurse);
> 
> 	     /* mf to zip */
> 	     strcpy (tempfn, "META-INF/manifest.mf");
> 	     sprintf (fullfn, "%s/%s", tree, tempfn);
> 	     JzipAdd(fullfn, tempfn, zipfile, compression_level);
> 
> 	     /* sf to zip */
> 	     sprintf (tempfn, "META-INF/%s.sf", base);
> 	     sprintf (fullfn, "%s/%s", tree, tempfn);
> 	     JzipAdd(fullfn, tempfn, zipfile, compression_level);
> 
>          if(verbosity >= 0) {
>            PR_fprintf(outputFD, "%s \n", XPI_TEXT);
> 		 }
>         
>       } else { /* Create regularly signed archive */
>       
>          if(zip_file) {
>            zipfile = JzipOpen(zip_file, NULL /*no comment*/);
>          }
> 
> 	     manifesto (tree, install_script, recurse);
>         
> 	     if (keyName)
>    	     {
>            status = create_pk7 (tree, keyName, &keyType);
>            if (status < 0)
>            {
>    			 PR_fprintf(errorFD, "the tree \"%s\" was NOT SUCCESSFULLY SIGNED\n", tree);
> 			 errorCount++;
>  			 exit (ERRX);
>    		   }
>     	 }
>  
> 	     /* mf to zip */
> 	     strcpy (tempfn, "META-INF/manifest.mf");
> 	     sprintf (fullfn, "%s/%s", tree, tempfn);
> 	     JzipAdd(fullfn, tempfn, zipfile, compression_level);
> 
> 	     /* sf to zip */
> 	     sprintf (tempfn, "META-INF/%s.sf", base);
> 	     sprintf (fullfn, "%s/%s", tree, tempfn);
> 	     JzipAdd(fullfn, tempfn, zipfile, compression_level);
>          
>          /* rsa/dsa to zip */
>          sprintf (tempfn, "META-INF/%s.%s", base, (keyType==dsaKey ? "dsa" : "rsa"));
>          sprintf (fullfn, "%s/%s", tree, tempfn);
>          JzipAdd(fullfn, tempfn, zipfile, compression_level);
91,105d150
<     }
< 
<   /* mf to zip */
< 
<   strcpy (tempfn, "META-INF/manifest.mf");
<   sprintf (fullfn, "%s/%s", tree, tempfn);
<   JzipAdd(fullfn, tempfn, zipfile, compression_level);
< 
<   /* sf to zip */
< 
<   sprintf (tempfn, "META-INF/%s.sf", base);
<   sprintf (fullfn, "%s/%s", tree, tempfn);
<   JzipAdd(fullfn, tempfn, zipfile, compression_level);
< 
<   /* rsa/dsa to zip */
107,109d151
<   sprintf (tempfn, "META-INF/%s.%s", base, (keyType==dsaKey ? "dsa" :
"rsa"));
<   sprintf (fullfn, "%s/%s", tree, tempfn);
<   JzipAdd(fullfn, tempfn, zipfile, compression_level);
Index: signtool/signtool.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/cmd/signtool/signtool.c,v
retrieving revision 1.9
diff -r1.9 signtool.c
82a83,84
> int xpi_arc = 0;
> 
136c138,139
<     TOKEN_OPT
---
>     TOKEN_OPT,
>     XPI_ARC_OPT
258a262,263
> 		} else if(!PL_strcasecmp(buf, "xpi")) {
>             type = XPI_ARC_OPT;
392a398,400
> 				case 'X':
> 					type = XPI_ARC_OPT;
> 					break;
793a802,804
>     case XPI_ARC_OPT:
> 		xpi_arc = 1;
> 		break;
891a903,913
>   /* -X needs -Z */
> 
>   if (xpi_arc && !zipfile)
>     {
>     PR_fprintf (errorFD, "%s: option XPI (-X) requires option jarfile (-Z)\n",
> 		PROGRAM_NAME);
> 	errorCount++;
> 	retval = -1;
> 	goto cleanup;
>     }
> 
Index: signtool/signtool.h
===================================================================
RCS file: /cvsroot/mozilla/security/nss/cmd/signtool/signtool.h,v
retrieving revision 1.6
diff -r1.6 signtool.h
78c78
< #define VERSION		"1.3"
---
> #define VERSION		"1.4 Beta - XPI Testing"
80c80
< 
---
> #define XPI_TEXT "Creating XPI Compatible Archive"
127a128
> extern int xpi_arc;
Index: signtool/util.c
===================================================================
RCS file: /cvsroot/mozilla/security/nss/cmd/signtool/util.c,v
retrieving revision 1.16
diff -r1.16 util.c
196c196
<   PR_fprintf(outputFD, "    -b\"basename\"\t\tbasename of .sf, .rsa files for
signing\n");
---
>   PR_fprintf(outputFD, "    -b \"basename\"\t\tbasename of .sf, .rsa files for signing\n");
198,203c198,203
<   PR_fprintf(outputFD, "    -d\"certificate directory\"\tcontains cert*.db
and key*.db\n");
<   PR_fprintf(outputFD, "    -e\".ext\"\t\t\tsign only files with this
extension\n");
<   PR_fprintf(outputFD, "    -f\"filename\"\t\t\tread commands from file\n");
<   PR_fprintf(outputFD, "    -G\"nickname\"\t\tcreate object-signing cert with
this nickname\n");
<   PR_fprintf(outputFD, "    -i\"installer script\"\tassign installer
javascript\n");
<   PR_fprintf(outputFD, "    -j\"javascript directory\"\tsign javascript files
in this subtree\n");
---
>   PR_fprintf(outputFD, "    -d \"certificate directory\"\tcontains cert*.db and key*.db\n");
>   PR_fprintf(outputFD, "    -e \".ext\"\t\t\tsign only files with this extension\n");
>   PR_fprintf(outputFD, "    -f \"filename\"\t\t\tread commands from file\n");
>   PR_fprintf(outputFD, "    -G \"nickname\"\t\tcreate object-signing cert with this nickname\n");
>   PR_fprintf(outputFD, "    -i \"installer script\"\tassign installer javascript\n");
>   PR_fprintf(outputFD, "    -j \"javascript directory\"\tsign javascript files in this subtree\n");
206c206
<   PR_fprintf(outputFD, "    -k\"cert nickname\"\t\tsign with this
certificate\n");
---
>   PR_fprintf(outputFD, "    -k \"cert nickname\"\t\tsign with this certificate\n");
209c209
<   PR_fprintf(outputFD, "    -m\"metafile\"\t\tinclude custom
meta-information\n");
---
>   PR_fprintf(outputFD, "    -m \"metafile\"\t\tinclude custom meta-information\n");
214c214
<   PR_fprintf(outputFD, "    -p\"password\"\t\tfor password on command line
(insecure)\n");
---
>   PR_fprintf(outputFD, "    -p \"password\"\t\tfor password on command line (insecure)\n");
219c219,221
<   PR_fprintf(outputFD, "    -x\"name\"\t\t\tdirectory or filename to
exclude\n");
---
>   PR_fprintf(outputFD, "    -x \"name\"\t\t\tdirectory or filename to exclude\n");
>   PR_fprintf(outputFD, "    -X\t\t\t\tCreate XPI Compatible Archive\n"
>             "\t\t\t\t(used in conjunction with the -Z)\n");
221c223
<   PR_fprintf(outputFD, "    -Z\"jarfile\"\t\t\tcreate JAR file with the given
name.\n"
---
>   PR_fprintf(outputFD, "    -Z \"jarfile\"\t\t\tcreate JAR file with the given name.\n"
238a241,242
>   PR_fprintf(outputFD, "%s -d \"certificate directory\" -k \"cert nickname\" -p \"password\" -X -Z \"file.xpi\" directory-tree\n", PROGRAM_NAME);
>   PR_fprintf(outputFD, "  Common syntax to create a XPInstall compatible signed archive\n\n");
Jeff, thanks for your work on this.  

I confirm this is an enhancement request.  :)

I take it that your comment 2 above was intended to be a revision to 
the patch you attached.  Is that right?  

If so, the way to do that is to *attach* the revised patch to the bug,
and then mark the previous attachment as "obsolete".  

Putting a patch inline in the bug commentary makes it difficult to use,
and should only be used when commenting on a small part of a patch.

Also, please use "diff -u"  (or "cvs diff -u") when creating patches.

In your posting in the newsgroup, you mentioned that you are now 
calling manfesto twice, which results in putting two copies of some 
files into the directories, and you wondered how to overcome that.

Creating two copies of the various files is a problem that would be 
serious enough that it would prevent the patch from being accepted.

The answer may be to split the "manifesto" function apart into two 
functions that can be called at separate times.  I have not studied 
this in enough depth to make a specific recomendation.  
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Priority: -- → P3
Version: unspecified → 3.9
Attachment #151772 - Attachment is obsolete: true
Updated patch, fixed indentations.
Attachment #151788 - Attachment is obsolete: true
(In reply to comment #3)
> Jeff, thanks for your work on this.  
> 
> I confirm this is an enhancement request.  :)
> 
> I take it that your comment 2 above was intended to be a revision to 
> the patch you attached.  Is that right?  

I got confused with the interface, I was trying to update it. I added the new
one as an attachment.


> 
> Also, please use "diff -u"  (or "cvs diff -u") when creating patches.

Will Do.

> In your posting in the newsgroup, you mentioned that you are now 
> calling manfesto twice, which results in putting two copies of some 
> files into the directories, and you wondered how to overcome that.
> 
> Creating two copies of the various files is a problem that would be 
> serious enough that it would prevent the patch from being accepted.
> 
> The answer may be to split the "manifesto" function apart into two 
> functions that can be called at separate times.  I have not studied 
> this in enough depth to make a specific recomendation.  

Ok, I did alot more looking through the way manifesto() and manifesto_fn() run.
Most of those two functions are meant for creating the manifest.mf and
zigbert.sf files. There is some other code in there that deals with the -J
options. Which cannot be used with -Z. I found that it involved very little to
actually make a new functions that will only add the files to the archive. All
the directory and file permissions were checked previously when manifesto() ran
so that part is already covered. 

The new patch added has this new method implemented. I have tested it with all
my test archives and it is behaving as expected. Archives are created exactly
the same as before with the -X not present. If -X is present it runs manifesto
before the zip file is set so the files are not added to the zip file. After
all the META-INF/ files are created and zigbert.rsa is added to the zip file,
the new function will run through all the files in the tree and add them to the
zip file.

Let me know what you think.
In news://news.mozilla.org:119/40DD0D6C.9020604@meer.net Doug Turner wrote:

> I did something like this to create the xpi (from a newsgroup posing I made):
> 
> signtool -d ./certs -kdougt test
> cd test
> zip test.xpi META-INF/zigbert.rsa
> zip -r -D test.xpi * -x META-INF/zigbert.rsa
> mv test.xpi ../
> cd .. 

So, there is a work-around for this bug, hence P3
Comment on attachment 151789 [details] [diff] [review]
New -X option for XPI archive creation -- Not using manifesto() twice.

Review comments on this third patch:

1) in general, this patch is much better than earlier ones.
   Don't worry about whitespace/indentation.  I intend to run the
   signtool sources through cb (c beautifier) prior to the NSS 3.10
   release.

2) We don't want to call the new code 1.3 or 1.4 Beta or 1.4.
   6 years ago, signtool was released as a stand-alone tool, 
   and so it had its own release notes and version number.  
   For the last 4 years, signtool has not been released separately from NSS.  
   It is just another one of the NSS tools, and NSS has its own release 
   numbers and release notes.  This new code will be part of NSS 3.10, 
   and so, if anything, this new version should be version 3.10.  

3) This patch duplicates a lot of code in sign.c so that it can reorder
   a couple of things.	It would be better if this patch didn't duplicate
   all that code.

   It appears to me that the differences between the xpi_arc code path
   and the previous code path are:
   a) whether manifesto is called immediately before or immediatel after
      the conditional call to Jzipopen.
   b) an additional conditional verbose output (could be in both paths), 
   c) whether the output of the .[dr]sa file happens just before the code
      marked with "mf to zip", or whether it happens just after the code 
      marked with "sf to zip", and
   d) whether there is an additional foreach loop just before "mf to zip".

Those differences could be accomplished with must less duplication.  For 
example (in pseudo code here):	
   a)  if (xpi_arc) 
	   manifesto
       JzipOpen
       if (!xpi_arc)
	   manifesto

    c) if (xpi_arc) {
	   jzipadd .[dr]sa
	   foreach
       }
       mf to zip
       sf to zip
       if (!xpi_arc)
	   jzipadd .[dr]sa

4) it would be good to put a block comment by each of those two places,
   explaining the significance of the ordering of the JzipOpen and manifesto 
   calls, and explaining why the extra foreach call is needed in the xpi_arc
   path.
Attachment #151789 - Flags: review-
Blocks: 249330
(In reply to comment #7)
> (From update of attachment 151789 [details] [diff] [review])
> Review comments on this third patch:
> 
> 1) in general, this patch is much better than earlier ones.
>    Don't worry about whitespace/indentation.  I intend to run the
>    signtool sources through cb (c beautifier) prior to the NSS 3.10
>    release.

Didn't even now a beautifier existed for C.  

> 2) We don't want to call the new code 1.3 or 1.4 Beta or 1.4.
>    6 years ago, signtool was released as a stand-alone tool, 
>    and so it had its own release notes and version number.  
>    For the last 4 years, signtool has not been released separately from NSS.  
>    It is just another one of the NSS tools, and NSS has its own release 
>    numbers and release notes.  This new code will be part of NSS 3.10, 
>    and so, if anything, this new version should be version 3.10.  

Thats fine. I named it that so I could differentiate which one I was testing.

> 3) This patch duplicates a lot of code in sign.c so that it can reorder
>    a couple of things.	It would be better if this patch didn't duplicate
>    all that code.
> 
>    It appears to me that the differences between the xpi_arc code path
>    and the previous code path are:
>    a) whether manifesto is called immediately before or immediatel after
>       the conditional call to Jzipopen.
>    b) an additional conditional verbose output (could be in both paths), 
>    c) whether the output of the .[dr]sa file happens just before the code
>       marked with "mf to zip", or whether it happens just after the code 
>       marked with "sf to zip", and
>    d) whether there is an additional foreach loop just before "mf to zip".
> 
> Those differences could be accomplished with must less duplication.  For 
> example (in pseudo code here):	
>    a)  if (xpi_arc) 
> 	   manifesto
>        JzipOpen
>        if (!xpi_arc)
> 	   manifesto
> 
>     c) if (xpi_arc) {
> 	   jzipadd .[dr]sa
> 	   foreach
>        }
>        mf to zip
>        sf to zip
>        if (!xpi_arc)
> 	   jzipadd .[dr]sa

That is better. Like I said before I'm not much a C programmer. Thanks for the
criticism. I am adding a new patch of my current code with this method. 

> 4) it would be good to put a block comment by each of those two places,
>    explaining the significance of the ordering of the JzipOpen and manifesto 
>    calls, and explaining why the extra foreach call is needed in the xpi_arc
>    path.  
> 
I had gone through and commented it all after I made the last patch but lost it
when DevC++ decided to crash on me and I gave up and submitted the uncommented
patch. I didn't have any more time to be at the computer. The new patch has more
comments in it.
No longer blocks: 249330
Attachment #151789 - Attachment is obsolete: true
taking bug
Assignee: wchang0222 → nelson
targetting NSS 3.10.
Target Milestone: --- → 3.10
Comment on attachment 152090 [details] [diff] [review]
-X option patch -- Using Nelson's proposed method

The XPI path in this patch depends on JzipAdd failing each and every time
it is called UNTIL JzipOpen is called (which is after manifesto has already
trafersed the tree once).  Given that the patch intends for JzipAdd to fail,
it would be good if JzipAdd failed quickly, without doing a lot of work 
first.	

So, I suggest an additional patch, to JzipAdd in zip.c, to cause it to 
detect that zipfp is NULL, and return immediately in that case, 
before it tries to do anything more.  I am giving this patch r+, 
conditional on the completion of the patch to JzipAdd.

Jeff, you can just add that patch as an additional attachment to this bug,
without needing to obsolete the present patch.	I am also going to ask 
for a second review of this patch.

Wan-Teh, I am planning to run the sources through cb after this, so I
have forgone the usual comments about coding style (since these sources
already have at least 4 distinct styles in them).  I would prefer to see
the version strings come from some NSS header file rather than being hard
coded in signtool.h (where they are now), but I haven't looked to see 
how to do that.
Attachment #152090 - Flags: superreview?(wchang0222)
Attachment #152090 - Flags: review+
Comment on attachment 152090 [details] [diff] [review]
-X option patch -- Using Nelson's proposed method

I don't understand why there is an extra foreach call
(to invoke manifesto_xpi_fn) in the xpi_arc case.
Does that add the files to the zip archive twice?
(Note: I am not familiar with signtool or zip archives.)

You can get the "3.10" string by including "nss.h"
and using the NSS_VERSION macro.  Use C's string
literal concatenation.	For example,

#define DEFAULT_COMMON_NAME "Signtool " NSS_VERSION " Testing Certificate"
#define CREATOR  "Signtool (signtool " NSS_VERSION ")"

>+ *  m a n i f e s t o _xpi_ f n

Nit: needs spaces between the three letters in "xpi".
Comment 13 tells me that the block comment that Jeff added just before the 
only call to "manifesto" was insufficient.  That comment needs to be 
expanded to explain that, if the zipfile itself is not opened prior to 
calling manifesto, manifesto will traverse the tree and generate all the
relevant signature files, but will not actually put any of them into the 
zip file (since it's not open).  Consequently, after that job is done, it
is necessary to take another pass over the tree to output the files in 
the desired order.  

Jeff, please expand the comment to say that.
Also, please use Wan-Teh's suggestion for using the string from nss.h
rather than hardcoding it, as the patch presently does.
You might as well submit yet another single patch that includes these
changes as well as the change to JzipAdd that I proposed in comment 12.
Comment on attachment 152090 [details] [diff] [review]
-X option patch -- Using Nelson's proposed method

Apparently Nelson understands how the patch works,
so I'll ask him to explain this patch to me.

Here is the status of my review of this patch: I
verified that it won't introduce regressions because
if xpi_arc is 0, the new code is equivalent to the
old code.  So in this sense the patch meets my
minimum requirement.  I also verified that the code
matches the comments, and the comments make sense to
me.  But I don't fully understand the changes in
sign.c yet.

I have a question: should we add a diagnostic
message to the beginning of manifesto_xpi_fn because
manifesto_fn has it?

    if(verbosity >= 0) {
	PR_fprintf(outputFD, "--> %s\n", relpath);
    }
Checked in Jeff's patch.  I plan to make additional changes soon.

Checking in README;      new revision: 1.2; previous revision: 1.1
Checking in sign.c;      new revision: 1.10; previous revision: 1.9
Checking in signtool.c;  new revision: 1.11; previous revision: 1.10
Checking in signtool.h;  new revision: 1.8; previous revision: 1.7
Checking in util.c;      new revision: 1.18; previous revision: 1.17
This patch changes signtool to get the version number string from nss.h,
and causes JzipAdd to shortcut when the zip file is not open.
It also extends the QA test script to test out signtool in this new mode.
Comment on attachment 153234 [details] [diff] [review]
Apply Wan-Teh and Nelson's review feedback

Wan-Teh, please review that this patch implements your suggestions correctly,
and that it tests the new -X option.
Attachment #153234 - Flags: review?(wchang0222)
Comment on attachment 153234 [details] [diff] [review]
Apply Wan-Teh and Nelson's review feedback

r=wtc.
Attachment #153234 - Flags: review?(wchang0222) → review+
Checking in cmd/signtool/sign.c; 
new revision: 1.11; previous revision: 1.10

Checking in cmd/signtool/signtool.h;
new revision: 1.9; previous revision: 1.8

Checking in cmd/signtool/util.c;
new revision: 1.19; previous revision: 1.18

Checking in cmd/signtool/zip.c;
new revision: 1.4; previous revision: 1.3

Checking in tests/tools/tools.sh;
new revision: 1.12; previous revision: 1.11
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Many thanks to Jeff Klawiter for contributing this fix!
Comment on attachment 153234 [details] [diff] [review]
Apply Wan-Teh and Nelson's review feedback

>Index: cmd/signtool/zip.c
>@@ -172,10 +173,12 @@
> 	if( !fullname || !filename || !zipfile) {
> 		return -1;
> 	}
> 
> 	zipfp = zipfile->fp;
>+	if (!zipfp)
>+		return -1;
> 
> 
> 	if( (readfp = PR_Open(fullname, PR_RDONLY, 0777)) == NULL) {
> 		char *nsprErr;
> 		if(PR_GetErrorTextLength()) {

Nelson, while I think this change is fine,
I don't understand why we weren't crashing
in PR_Seek(zipfp) or PR_Write(zipfp) before
if zipfp was NULL.
(In reply to comment #21)
> Many thanks to Jeff Klawiter for contributing this fix!


You're welcome. I'm sorry I wasn't able to finish up the patch with the rest of
the comments made. Things have been really busy in the real world lately and
I've barely even had the time to check my email for the last couple weeks.
Either way I'm happy this new option is being added to signtool and look forward
to the next official release of NSS.
Attachment #152090 - Flags: superreview?(wchang0222)
Can anyone please e-mail me a Windows version of the new signtool?

Note: I don't have a compiler at hand (6000 miles from home) and desperately
need to sign one of my XPI's, thank you.

/HJ
See
http://groups-beta.google.com/group/netscape.public.mozilla.crypto/msg/2f15d0abe608bcd6.
If you have further questions, please follow up in the newsgroup.
Google's web-based interface to newsgroups is a great way to search news
archives, but AFAIK, messages posted there do not propagate to mozilla's
own news server, and so may not be seen by mozilla developers.  So if you 
want to post messages into the mozilla crypto newsgroup, I suggest that 
you use mozilla's news server directly.  This URL should get you there.
news://news.mozilla.org:119/netscape.public.mozilla.crypto
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: