379633 - Scriptable zipwriter component

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Dave Townsend [:mossop]]

This is kind of a follow on from bug 338987 however the code I have has essentially been rewritten with what I think is a simpler API and providing more benefits.

The code that I will attach soon implements a component that can load existing zip files and add/remove files to them, or just start with a blank zip file.

It also implements a deflate stream converter which it uses internally to perform compression when requested for files.

Comment 1

[Mike Kaply [:mkaply]]

Woohoo!

Comment 2

[Dave Townsend [:mossop]]

Attachment: implementation rev 1

Here's the first version ready for review.

This currently goes in mozilla/extensions/zipwriter though presumably somewhere more sensible might be found for it.

This includes a bunch of testcases.

I'm not sure it's worth me giving a full blow by blow description of the code here, but I can do if necessary.

Comment 3

[Dave Townsend [:mossop]]

Attachment: implementation rev 1

Oops, fix the comments in some places.

Comment 4

[Dave Townsend [:mossop]]

For those that don't know, you can build with this component by using the following in your mozconfig:

ac_add_options --enable-extensions=default,zipwriter

Comment 5

[Alex Vincent [:WeirdAl]]

First thoughts, not having taken more than a cursory glance at the patch:

* Talked with Mossop over irc about undoing an action before committing (like add a file, remove the file, commit).  He's indicated his zipwriter passes the testcase I suggested, and a future patch will include that testcase.

* I wonder if there should be some allmakefiles.sh love.

* nsIZipWriter (the interface) could be useful with other compression algorithms (tar, gz, bz2), while the component itself would handle only zip.  I don't see this as a problem, but I would suggest the type of compression format be included in the component contract id somehow.  (Of course, that would necessitate zipreader components for the other compression formats as well, if and when someone implements them.)

Comment 6

[Jeff Walden [:Waldo]]

Things noted in a read-through (not super-intensive):

- make the macros inline functions, for type-safety, not evaluating
  parameters multiple times (this happens numerous places in the patch, and
  compilation-unit boundaries make it possible not all can be caught by the
  compiler), not requiring extra parenthesization, etc.
- variable declarations at the point of first use, not the C89
  at-the-top-of-block style, when possible
- crc_table might want to be PRUint32[] since it looks like you're relying
  on a particular width (if it's staying, sounds like it might not be)
- crc_table's origins should at least be commented with a pointer to their
  source for doc purposes, if it stays
- NS_ERROR_IN_PROGRESS is more informative than NS_ERROR_FAILURE if it's not
  abusing another module's nsresults too much
- rearrange members of classes to move larger variables to the beginning;
  mixing PRUint32, PRUint16, PRUint8 is a good way to bloat classes with
  padding
- use PRPackedBool for class members, not PRBool (or bitfields if you have
  many booleans, but I didn't remember such a place)
- I see a bunch of uses of NS_Alloc/NS_Free for data used only within a
  single method; I'd prefer new/delete for this, in general, since that's
  less Mozilla-specific knowledge (stack-based would be even nicer, but
  it's possibly-large arrays, and only C99 has variable-size stack arrays,
  methinks)
- addDirectoryEntry and addFile have out-of-date generated IDL comments in
  the implementation
- need to null-check do_CreateInstance (and NS_Alloc, as later noted by
  Mook)
- I'd prefer nsZipWriter::BeginProcessingNextItem's branches of execution as
  separate functions (inlined, perhaps, if the compiler is capable of doing
  so in response to an |inline|)
- BeginProcessingNextItem is tail-recursive; GCC might get that right
  (maybe), but you can't rely on the compiler to get it right, so you risk
  stack overflow
- nit: I prefer struct over class for POD types like nsZipQueueItem
- implicit global variables in head_zipwriter.js aren't the best style, in
  my opinion (except for functions, Components.Constructor returns, and
  Cc/Ci/etc.)
- consider using http://developer.mozilla.org/en/docs/Components.Constructor
- for the |do_throw("shouldn't be able to add an existing entry");| test,
  the catch for it is empty -- test for a specific exception, even if not
  specified in the interface (probably should be), to ensure changes to
  the exception are intentional
- many magic numbers in the tests should be consts with descriptive names

Comment 7

[Dave Townsend [:mossop]]

Comment on attachment 263780 [details] [diff] [review]
implementation rev 1

Going to address a bunch of stuff from the earlier comments so cancelling review for now.

Comment 8

[Dave Townsend [:mossop]]

Attachment: implementation rev 2

This is an updated version that addresses the majority of the previous comments, adds some more testcases and fixes a problem that would have left certain zip files corrupt.

The only comment that wasnt addressed was about the global variables in head_zipwriter.js. These are necessary in my opinion in order that any opened zip file can be correctly closed and deleted in tail_zipwriter.js

Comment 9

[Dave Townsend [:mossop]]

Attachment: implementation rev 3

Updated patch, most changes suggested by Mook in a review over IRC.

Fixes some issues with ioflags on Open. Fixes an issue caused by assuming that nsIFile.lastSize was a PRTime. Performs better cleanup in the event of a failure in the middle of an operation (though the resultant zip file will end up pretty much corrupt). Removes some potential leaks. Gets rid of lots more magic numbers. Use helper functions for creating inputstreams etc.

Adds more testcases for problems during asynchronous operation and directory adding.

Comment 10

[Jeff Walden [:Waldo]]

|delete foo| does the right thing if |foo| is null, so unconditionally deleting is fine if you null it out afterward.  You also have at least a couple instances of |X* a = new X[N]; if (!a) { delete [] a; ... }| where the delete is just visual clutter (any decent compiler would have optimized it away).


Style nit: add whitespace around binary operators for better readability, in other words |x += 3;| instead of |x+=3;|.


You have the following pattern several places in the code:

> if (FAILED(some operation))
>   maybe do cleanup operation X and then return;
> if (FAILED(some second operation))
>   maybe do cleanup operation X and then return;

The visual clutter can be reduced by the following transformation:

> if (FAILED(some operation) ||
>     FAILED(some second operation))
>   maybe do cleanup operation X and then return;


In nsZipWriter::OnFileEntryComplete you have quite a few error cases of |Cleanup(); return;|.  If you structure the code right, you could simplify these substantially with well-placed gotos to a label, although I think you'd have to tweak local variable declarations so that you're not jumping over constructor execution (I think C++ forbids this, but I could be wrong).  This might also be a place to apply the previous note, although for reader sanity I'm not sure you really want the ifs to be too long.


The new function findError doesn't seem to be used anywhere; maybe a missed file?

Comment 11

[Dave Townsend [:mossop]]

(In reply to comment #10)
> |delete foo| does the right thing if |foo| is null, so unconditionally deleting
> is fine if you null it out afterward.

I could only figure that you were referring to ~nsZipHeader here so I've corrected that.

> You also have at least a couple
> instances of |X* a = new X[N]; if (!a) { delete [] a; ... }| where the delete
> is just visual clutter (any decent compiler would have optimized it away).

I could only find one instance of this, but corrected that too.

> Style nit: add whitespace around binary operators for better readability, in
> other words |x += 3;| instead of |x+=3;|.

Yeah bad habit from another code style I'm used to.

> You have the following pattern several places in the code:
> 
> > if (FAILED(some operation))
> >   maybe do cleanup operation X and then return;
> > if (FAILED(some second operation))
> >   maybe do cleanup operation X and then return;
> 
> The visual clutter can be reduced by the following transformation:
> 
> > if (FAILED(some operation) ||
> >     FAILED(some second operation))
> >   maybe do cleanup operation X and then return;
> 
> 
> In nsZipWriter::OnFileEntryComplete you have quite a few error cases of
> |Cleanup(); return;|.  If you structure the code right, you could simplify
> these substantially with well-placed gotos to a label, although I think you'd
> have to tweak local variable declarations so that you're not jumping over
> constructor execution (I think C++ forbids this, but I could be wrong).  This
> might also be a place to apply the previous note, although for reader sanity
> I'm not sure you really want the ifs to be too long.

Not entirely sure about these points. Will have to sit down and look through on a case by case basis.

> The new function findError doesn't seem to be used anywhere; maybe a missed
> file?

Thats a mistake, left in some debug code I was using. Will remove it.

I won't bother to submit a new version of the patch with these changes right now.

Comment 12

[Dave Townsend [:mossop]]

Attachment: implementation rev 4

The previous version had some bad issues dealing with modification times across timezones so this fixes it. It also updates the testcases for moving the data into it's own subdirectory, and relies on working modification times coming from zipreader (so needs bug 379841 for them to pass). Also addresses some of the nits from the previous.

Comment 13

[Benjamin Smedberg]

Comment on attachment 264025 [details] [diff] [review]
implementation rev 4

General notes: please de-tab

>Index: extensions/zipwriter/Makefile.in

I'm tired of the extensions/ dumping-ground. Can we put this in modules/libjar/zipwriter?

>Index: extensions/zipwriter/public/nsIZipWriter.idl

>+  /**
>+   * Returns a nsIZipEntry describing a specified zip entry or null if there
>+   * is no such entry in the zip file
>+   *
>+   * @param aZipEntry the path of the entry
>+   */
>+  nsIZipEntry getEntry(in AUTF8String aZipEntry);
>+
>+  /**
>+   * Checks whether the zipfile contains an entry specified by zipEntry.
>+   *
>+   * @param aZipEntry the path of the entry
>+   */
>+  boolean hasEntry(in AUTF8String aZipEntry);

These two methods are exact mirrors of nsIZipReader methods, right? Do you think there is any value in a common base interface shared by the two?

>+  /**
>+   * Adds a new directory entry to the zip file. This operation completes
>+   * immediately.
>+   *
>+   * @param aZipEntry the path of the directory entry
>+   * @param aModTime the modification time of the directory in microseconds
>+   *
>+   * @return the nsIZipEntry describing the stored directory.
>+   *
>+   * @throws NS_ERROR_NOT_INITIALIZED if no zip file has been opened
>+   * @throws NS_ERROR_FILE_ALREADY_EXISTS if the path alread exists in the
>+   *  file
>+   * @throws NS_ERROR_IN_PROGRESS if another operation is currently in progress
>+   */
>+  nsIZipEntry addDirectoryEntry(in AUTF8String aZipEntry, in PRTime aModTime);

There seems to be some unfortunate logic mismatch between the synchronous API and the async API.

This method in particular takes a name + modtime, while the async API takes a nsIFile which happens to be a directory. Is there any reason you couldn't have a single API with a sync/async flag, e.g.:

  void addEntryDirectory(in AUTF8String aZipEntry, in PRTime aModTime, in boolean aQueue);
  void addEntryStream(in AUTF8String aZipEntry, in PRTime aModTime, in nsIChannel data,
                     in boolean aQueue);
  void addEntryFile(in AUTF8String aZipEntry, in nsIFile data, in boolean aQueue);
  void removeEntry(in AUTF8String aZipEntry, in boolean aQueue)

Note that this does switch from using an outputstream to a channel: if in sync mode it would call channel.open, and if in async it would call channel.asyncOpen. It also removes the nsIZipEntry return values because I'm having trouble seeing how they are valuable. But you could certainly keep them and return null in the async case.

I'm not sure whether this is really hard to do, or whether this is a case of "perfect as the enemy of the good"... tell me what you think.

>Index: extensions/zipwriter/src/Makefile.in

>+LIBXUL_LIBRARY        = 1

LIBXUL_LIBRARYies require changes to toolkit/library as well...

>Index: extensions/zipwriter/src/nsDeflateConverter.cpp

>+NS_IMETHODIMP nsDeflateConverter::OnDataAvailable(nsIRequest *aRequest,
>+                                                  nsISupports *aContext,
>+                                                  nsIInputStream *aInputStream,
>+                                                  PRUint32 aOffset,
>+                                                  PRUint32 aCount)
>+{
>+    if (!mListener)
>+        return NS_ERROR_NOT_INITIALIZED;
>+
>+    char* buffer = new char[aCount];

Please use nsAutoArrayPtr to avoid the manual delete[] calls here and in 

nsZipReader::ReadCDSHeader
nsZipOutputStream::OnDataAvailable

>Index: extensions/zipwriter/test/Makefile.in

>+check::

What's the empty check target for? I don't think this is necessary.

Comment 14

[Dave Townsend [:mossop]]

Attachment: implementation rev 5

Next round. Addressed all the previous comments.

Comment 15

[Alex Vincent [:WeirdAl]]

Comment on attachment 269739 [details] [diff] [review]
implementation rev 5

More drive-by comments.  (Note these comments actually sound harsher than I intended - sorry.  This is in no way an official review, and you're free to ignore everything I've said here if you so choose.)

I saw a couple instances of "till" in here.  Please don't end up on bug 106386.  :)

>Index: modules/libjar/zipwriter/src/nsZipWriter.cpp
>+nsresult nsZipWriter::ReadFile(nsIFile *aFile)
>+{
...
>+    // Will never reach here in reality
>+    return NS_ERROR_UNEXPECTED;

Add a NS_NOTREACHED() perhaps?  (Sure, the loop can't be broken out of... for now.)

>+NS_IMETHODIMP nsZipWriter::AddEntryDirectory(const nsACString & aZipEntry, PRTime aModTime, PRBool aQueue)
>+    if (aQueue) {
...
>+        return NS_OK;
>+    }
>+    else {

Else after return?  http://beaufour.dk/jst-review/?patch=https%3A%2F%2Fbugzilla.mozilla.org%2Fattachment.cgi%3Fid%3D269739

You put an else after a return at least twice more.  I'm surprised the jst-review simulacrum only caught one of them.

>+NS_IMETHODIMP nsZipWriter::RemoveEntry(const nsACString & aZipEntry, PRBool aQueue)
>+                while (count > 0) {
>+                    if (count < sizeof(buf))
>+                      read = count;
>+                    else
>+                      read = sizeof(buf);

Inconsistent spacing.

>Index: modules/libjar/zipwriter/test/unit/tail_zipwriter.js
>+try {
>+  zipW.close();
>+}
>+catch (e) {
>+}

I really dislike empty catch blocks.  They don't tell me why you're catching the error.  Even a |// do nothing| comment would be better.  Besides, as I noted above, if ReadFile() returns NS_ERROR_UNEXPECTED (and it propagates all the way out, which I'm assuming it does), this will silently dump that on the floor.

In test code in particular, I'd think you'd never want this to throw an exception after the test is complete.  Please explain why you would want that, if I've misunderstood.

>Index: modules/libjar/zipwriter/test/unit/test_asyncadd.js
>+function run_test()
>+{
>+  //do_throw("");

Why leave this commented out and in the patch?

>Index: modules/libjar/zipwriter/test/unit/test_asyncbadadd.js
>+const FILENAME = "missing.txt"

Semicolon at the end of the line, please?  (Even though it's not necessary.)  Ditto for the following file:
>Index: modules/libjar/zipwriter/test/unit/test_asyncbadremove.js

>Index: modules/libjar/zipwriter/test/unit/test_editexisting.js
>+  try {
>+    zipW.removeEntry(BADENTRY, false);
>+    do_throw("shouldn't be able to remove a non-existant");
>+  }

Bug 106386 again ("non-existent").  Also, a non-existent what?

>Index: modules/libjar/zipwriter/test/unit/test_storedata.js
>+  var diff = Math.abs((entry.lastModifiedTime/PR_USEC_PER_MSEC) - time);

Suggest a space before and after the slash here, for readability.

>Index: modules/libjar/zipwriter/test/unit/test_undochange.js
>+function run_test()
>+{
>+  try {
>+    var source = do_get_file(DATA_DIR + TESTS[0]);
>+    zipW.addEntryFile(TESTS[0], Ci.nsIZipWriter.COMPRESSION_NONE, source, false);
>+    do_throw("Should not be able to add the same file twice");
>+  }
>+  catch (e) {
>+  }

You're not even checking the error type here.  Previous tests have checked that.

Comment 16

[Dave Townsend [:mossop]]

Attachment: implementation rev 6

Thanks for the comments, this addresses all those and I've fixed everything from the patch reviewer that I think are real issues.

Comment 17

[Benjamin Smedberg]

Comment on attachment 269848 [details] [diff] [review]
implementation rev 6

>Index: modules/libjar/zipwriter/public/Makefile.in

>+EXPORTS = $(NULL)

Don't even bother... remove this line.

>+XPIDLSRCS = nsIZipWriter.idl \
>+            $(NULL)

In this and many places below, please use the following style:

XPIDLSRCS = \
  nsIZipWriter.idl \
  $(NULL)

(that's a two-space indent)

>Index: modules/libjar/zipwriter/src/nsZipDataStream.cpp

>+#define TBLS 1
>+#include "../../../zlib/src/zutil.h"
>+#include "../../../zlib/src/crc32.h"

This is not right. You want a makefile var (ZLIB_REQUIRES I think?) to make sure you pick up the correct version of the zlib headers and whatnot.

I think this warrants a second-review... biesi if he's available, or maybe Neil.

Comment 18

[Dave Townsend [:mossop]]

Attachment: implementation rev 7

Tidied up the makefiles. Removed the hack to get at the crc tables by just using zlib's crc32 implementation (duh!). Given that that's only a 2 line change I'll assume the reviews still good.

Comment 19

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 269892 [details] [diff] [review]
implementation rev 7

Hmm, most code doesn't put the name of the original author on its own line

modules/libjar/zipwriter/public/nsIZipWriter.idl

Use forward declarations instead of #includes

You should document whether it's necessary to call addEntryDirectory for adding entries in a subdir

Documenting the format for entry names would be good too - do they have to start with a slash, do they use slashes or backslashes, etc


Hm... I wonder if it would be a good idea to have an add function that returns an nsIOutputStream that the caller could write into

modules/libjar/zipwriter/src/StreamFunctions.cpp
+ * Fully reads the required amount of data. Keeps reading untill all the

until only has one l

ZW_ReadData needs to handle the case when Read returns success and a 0 count (that indicates EOF)

modules/libjar/zipwriter/src/StreamFunctions.h
+  val += ((PRUint16)buf[(*off)++] & 0xff) << 8;

I'd use |= here, since you are really doing bit manipulation here, not addition as such

The functions here should be marked NS_HIDDEN

modules/libjar/zipwriter/src/nsDeflateConverter.cpp
+            // buffer is full, time to write it to disk!

this doesn't necessarily write do disk, it just passes it on to the listener...

modules/libjar/zipwriter/src/nsZipDataStream.cpp
+        nsIStreamConverter* converter = new nsDeflateConverter(aCompression);
+        NS_ENSURE_TRUE(converter, NS_ERROR_OUT_OF_MEMORY);
+
+        rv = converter->AsyncConvertData("uncompressed", "deflate", mOutput,
+                                         nsnull);

you shouldn't call functions on objects that have a zero refcount. I'd just store it in an nsCOMPtr.

+    }
+    else
+        mHeader->mMethod = ZIP_METHOD_STORE;

hm, I sort of think it looks better to have braces around the else too, if the then-branch has them

I'd have appreciated some comments about what the classes here are for, especially nsZipDataStream

+    if (NS_FAILED(rv)) {
+        mWriter->EntryCompleteCallback(mHeader, rv);
+    }
+
+    rv = CompleteEntry();
+    rv = mWriter->EntryCompleteCallback(mHeader, rv);

so you're calling the callback twice in some cases?

+    mHeader->mCRC = crc32(mHeader->mCRC, (const unsigned char*)aBuffer, aCount);

I'd prefer a C++ cast here

modules/libjar/zipwriter/src/nsZipHeader.cpp
+/* readonly attribute unsigned long size; */
+NS_IMETHODIMP nsZipHeader::GetSize(PRUint32 *aSize)
+{
+    NS_ENSURE_ARG_POINTER(aSize);

nullchecking out params is a complete waste of codesize and cpu cycles IMO

(not just in this function)

+    mComment = NS_LITERAL_CSTRING("");

mComment.Truncate();

(twice)

It would be sort of nice if you could describe the overall structure of a zip file somewhere, so that people reading this code have an idea what a CDS header is, or a EOCDR header.

Would be nice if BeginProcessingAddition didn't have to duplicate code from the individual Add* functions

Comment 20

[Jeff Walden [:Waldo]]

(In reply to comment #19)
> It would be sort of nice if you could describe the overall structure of a zip
> file somewhere, so that people reading this code have an idea what a CDS header
> is, or a EOCDR header.

I'd assume most people working with this code will have stumbled upon <http://mxr.mozilla.org/mozilla/source/modules/libjar/appnote.txt>, but it probably wouldn't hurt to mention it in comments.

Comment 21

[Dave Townsend [:mossop]]

(In reply to comment #19)
> (From update of attachment 269892 [details] [diff] [review])
> Hmm, most code doesn't put the name of the original author on its own line

According to the boilerplate (http://www.mozilla.org/MPL/boilerplate-1.1/mpl-tri-license-c), on a new line is the way to go.

> Hm... I wonder if it would be a good idea to have an add function that returns
> an nsIOutputStream that the caller could write into

I don't disagree, in fact an earlier version of the patch had one, however it's reasonably easy to set this up with a pipe and I think right now it'd be good to get this landed before thinking about adding that.

> Would be nice if BeginProcessingAddition didn't have to duplicate code from the
> individual Add* functions


I don't really see that there is much code duplicated at all between the functions, a few lines maybe, not worth doing this I think.

Comment 22

[Dave Townsend [:mossop]]

Attachment: implementation rev 8

This fixes all the other issues mentioned by biesi and adds some additional comments to the idl and classes including one with a brief outline of the zip structure and a link to the full spec.

This patch also adds a configure argument to enable/disable the component and marks it as default off.

Comment 23

[Benjamin Smedberg]

Comment on attachment 274660 [details] [diff] [review]
implementation rev 8

I don't think you need the AC_DEFINE... the AC_SUBST should be sufficient.

Comment 24

[Dave Townsend [:mossop]]

Attachment: ready for checkin

Made that minor change and biesi gave his sr over IRC with a drop to the indent on the license blocks.

Comment 25

[Mike Kaply [:mkaply]]

Are people aware of this comment in 

http://lxr.mozilla.org/seamonkey/source/LEGAL

 34 C) Stac, Inc., and its licensing agent Hi/fn, own several patents which
 35 disclose data compression methods implementing an LZS compression
 36 algorithm, including U.S. Patent Nos. 4,701,745 and 5,016, 009 ("the
 37 Stac Patents"). The Netscape Communicator code does not perform
 38 compression. If you modify the Netscape source code to perform
 39 compression, please take notice of the Stac Patents.

Comment 26

[Dave Townsend [:mossop]]

(In reply to comment #25)
> Are people aware of this comment in 
> 
> http://lxr.mozilla.org/seamonkey/source/LEGAL

My belief is that nothing in this code should have any effect on that statement. The compression code I call is the deflate code already in tree in zlib. What's more cairo, nss and libimg already call the same code. However I guess it might be worth getting some kind of legal input on it.

Comment 27

[Dave Townsend [:mossop]]

Comment on attachment 274672 [details] [diff] [review]
ready for checkin

Seeking approval to land this as is. It is not part of the default build so no risk to any of the current apps.

I'm speaking to legal about the issue in comment 25 and won't actually land unless it's resolved.

Comment 28

[Alex Vincent [:WeirdAl]]

Relevant references:
http://www.uspto.gov/main/faq/p120010.htm

Patent 4,701,745 (utility) filed March 3, 1986, granted October 20, 1987 (I think).  17 years after the grant is October 20, 2007 (really close).  20 years after utility is March 3, 2006.  In other words, if I'm reading this right, this one is about to go out of force.

As for the other one, patent 5016009, filed January 13, 1989, granted May 14, 1991.  Normally, I'd say "Uh-oh."  But get this:
http://www.uspto.gov/go/og/2003/week27/patexpi.htm

It's listed as an expired patent.  A petition has been filed for some reason - search on https://ramps.uspto.gov/eram/patentMaintFees.do.

IANAL nor a paralegal (I'm an engineer), but this might be good information for the guys in legal to think about.  It didn't take me very long to find.

Comment 29

[Alex Vincent [:WeirdAl]]

Sorry, I forgot to include the patent search link:
http://patft.uspto.gov/netahtml/PTO/srchnum.htm

Comment 30

[Dave Townsend [:mossop]]

Attachment: updated for trunk [checked in]

This is updated for trunk. No code changes but required moving around for allmakefiles.sh. This also now enables zipwriter by default for all except the embedding profiles. Because of the change in behaviour re-requesting review and will re-request approval to land after the freeze.

Comment 31

[Dave Townsend [:mossop]]

Comment on attachment 280789 [details] [diff] [review]
updated for trunk [checked in]

Seeking approval to land this for M9. This is now enabled for all regular apps but as an unused component shouldn't impact any app or content however provides a much wanted feature for extension developers and application developers.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Are there any security concerns here?  That is, is it possiblem for an extension to shoot itself in the foot by passing insufficiently-vetted args to this component?

Comment 33

[Dave Townsend [:mossop]]

(In reply to comment #32)
> Are there any security concerns here?  That is, is it possiblem for an
> extension to shoot itself in the foot by passing insufficiently-vetted args to
> this component?

I can't think of anything that might be what I'd consider a security problem. About the only things you can pass to the component other than simple strings are channels, inputstreams and files. For all these all the component takes an interest in is the data it can pull out of those. So unless there is some issue with some sensitive data that an extension can already get hold of I can't think of a problem.

Sure if an extension wanted to be malicious it could attempt to overwrite it's own, or Firefox's jar files with it, but they can do that already, and there are easier ways to override some of the app's chrome using a chrome override for instance. The component can also be used to create xpi files that could be installed as extensions (actually one of it's use cases), but again if someone wanted to be malicious there are easier ways to install a rogue extension into the browser.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 280789 [details] [diff] [review]
updated for trunk [checked in]

OK.  Just making sure there are no situations where a zip file constructed from untrusted data (e.g. from a website) can cause crashes or whatnot.  That is, that the internal structure of the data passed to this code is irrelevant and that it's automatically self-consistent.

If that's the case, a=bzbarsky.

Comment 35

[Dave Townsend [:mossop]]

This was checked in however is showing test failures on windows and linux so I've disabled it by default for the time being. I have a good idea what's going on but need to double check it tomorrow

Comment 36

[Dave Townsend [:mossop]]

Attachment: follow-up patch

This is a follow up patch to correct 3 issues:

Needed to add the interfaces to the packaging.

The test failures are caused by nsIFile's stat cache, cloning the file gives an updated value which shows the tests passing.

Spotted a potential problem. Since nsIFile is mutable we should clone a copy to retain for the zip file and any files added to the queue. This stops callers from altering the value we have.

Comment 37

[Dave Townsend [:mossop]]

Comment on attachment 281180 [details] [diff] [review]
follow-up patch

Seeking approval, this fixes the unit tests, re-enables testing, packages the component correctly and corrects a potential issue in the component.

Comment 38

[Dave Townsend [:mossop]]

Final patch in and all tests passing now, looks like this is done :)

Comment 39

[Mike Kaply [:mkaply]]

Is someone going to create a page on developer.mozilla.org documenting this?

Comment 40

[Dave Townsend [:mossop]]

yes there could do with being some docs put up

Comment 42

[Eric Shepherd [:sheppy]]

For some reason this code is not in my pulled copy of the trunk tree.  I'm trying to figure out what's wrong.  It's coming up very soon on my plate of things to document.

Comment 43

[Chuck Baker]

I would love to incorporate this component into a couple of my extensions (FEBE and CLEO) but I can't find any useful documentation anywhere.  The code at http://mxr.mozilla.org/seamonkey/source/modules/libjar/zipwriter/public/nsIZipWriter.idl is too scant and cryptic to be of much help, and the nsIZipWriter Interface Reference at http://unstable.elemental.com/mozilla/build/latest/mozilla/modules/dox/interfacensIZipWriter.html times out when loading.  A Google code search for "nsIZipWriter" turns up nothing.

Do any docs, tutorials, examples, etc. exist yet?

Comment 44

[Nickolay_Ponomarev]

AFAIK, no documentation is written yet, but you can use the tests as examples:

http://mxr.mozilla.org/seamonkey/source/modules/libjar/zipwriter/test/unit/

Comment 45

[Chuck Baker]

Any word on the documentation yet?  I'm making some progress using the above test examples but it's frustratingly slow going.  The part that has me stumped now is how to incorporate the nsIObserver for the queue processing ... very little in the test examples that I can find.

Comment 46

[Dave Townsend [:mossop]]

nsIObserver is not used in this component. Possibly you are referring to nsIRequestObserver. As the comment says it is notified at the start and end of processing. Using the methods onStartRequest and onStopRequest

Comment 47

[Chuck Baker]

Attachment: javascript implementation of zipWriter

Comment 48

[Chuck Baker]

OK, I have been successful in creating an archive.  All works as it should *except* adding files synchronously.  Fx 3.0b2 generates a "Crash ! Bang! Boom!" error and shuts down.  I don't know if it's my code or a bug in zipWriter.  I've included my code as an attachment.

Comment 49

[Boris Zbarsky [:bzbarsky]]

Chuck, do you have a breakpad incident ID for that crash, by any chance?

Comment 50

[Chuck Baker]

I'm not sure what a "breakpad incident ID" is, but the crash was submitted  through talkback each time.  I can easily generate another crash if that would be helpful.  Just tell me what to look for.

Comment 51

[(not currently active) Ted Mielczarek]

Breakpad is what's underneath the new crash reporter, we don't have talkback anymore.  If you had a recent nightly build (or wait for b3), there's an about:crashes page that would let you find your reports.

Comment 52

[Boris Zbarsky [:bzbarsky]]

And in b2, you can follow the directions at http://kb.mozillazine.org/Breakpad#Location_of_crash_reports to find the submitted crash reports in question; those files will contain links to the website that has the stack and such.

Comment 53

[Chuck Baker]

Crash ID: bp-178b77ca-d1de-11dc-b2fd-001a4bd43ef6

Comment 54

[Dave Townsend [:mossop]]

(In reply to comment #53)
> Crash ID: bp-178b77ca-d1de-11dc-b2fd-001a4bd43ef6

Could you please file a new bug for this.

Comment 55

[Eric Shepherd [:sheppy]]

Documented here: http://developer.mozilla.org/en/docs/nsIZipWriter

Comment 56

[Chuck Baker]

Very nice!  I noticed that there was no testArchive() method mentioned.  Was this an oversight or is it not implemented?

Comment 57

[Dave Townsend [:mossop]]

(In reply to comment #56)
> Very nice!  I noticed that there was no testArchive() method mentioned.  Was
> this an oversight or is it not implemented?
> 

There was never any testArchive method on this component. In fact I'm don't see anywhere that the suggestion was ever mentioned.

Comment 58

[Chuck Baker]

Ok.  Considered it mentioned :)

Comment 59

[Dave Townsend [:mossop]]

nsIZipWriter does no extraction which is what testing would require. The nsIZipReader interface already has test functionality.