676179 - memory allocation/deallocation mismatch in other-licenses/7zstub/src/7zip/Common/StreamObjects.h

Status: NEW

Description

[David Volgyes]

Created attachment 550296 [details] [diff] [review]
proposed fix.

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110622232440

Steps to reproduce:

I run cppcheck 1.49 on trunk, and it found several bugs.
This is one of them.


Actual results:

In other-licenses/7zstub/src/7zip/Common/StreamObjects.h the code allocates memory with new[], but releases with delete.


Expected results:

The memory should be released with delete[]

Comment 1

[Josh Matthews [:jdm]]

Taras, I feel like you're one of the few people who has any stake in this code.

Comment 2

[:aceman]

Hi, the patch seems logical, have you confirmed it can cause a leak? The keywords are set indicating that.

Comment 3

[(dormant account)]

Comment on attachment 550296 [details] [diff] [review]
proposed fix.

nice

Comment 4

[:aceman]

(In reply to comment #0)
> I run cppcheck 1.49 on trunk, and it found several bugs.
> This is one of them.

Have you filed the other problems somewhere?

Comment 5

[Josh Matthews [:jdm]]

Yes, they are scattered hither and yon.

Comment 6

[David Volgyes]

(In reply to comment #4)
> (In reply to comment #0)
> > I run cppcheck 1.49 on trunk, and it found several bugs.
> > This is one of them.
> 
> Have you filed the other problems somewhere?

Actually, cppcheck showed 69 leaks in the code a few days ago. 
(45 memory leak, 24 resource leak (not closed file, etc.))
Some of them are false positive. I try to hunt the real ones one-by-one, and file a bug report for every individual bug. (I am not familiar with the firefox structure, and if I indicate two error in one report, you will not be able to assign them to two different components.) So some of them are already reported, and I work on the rest.

But this is a very simple process, anyone can do it with some time, so I encourage Mozilla to use cppcheck as a daily test, because it's very powerful tool.  (However, it has some false positive reports, especially when pointer==nsnull test happens, or where a factory method is used.)

When I have time, I report as many bug as I can, but there are a few hundreds (few dozen leak, and a lot of possible null pointer dereference and others), so it will take a while.

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Might be useful to have a tracking bug for all these that could be used e.g. to evaluate the usefulness of the tool.

Comment 8

[:aceman]

I could do the basic work, manage that tracking bug and link all those individual bugs to it. We'll see how many real problems come up. But I am not a mozilla developer, whom do you recommend I should CC in the tracking bug to see the results and usefulness of the tool?

Comment 9

[Benjamin Smedberg]

The z7 stub is the installer, and I believe that it is upstream code, not ours. I don't think this should be checked into our tree first.

Comment 10

[Robert Strong [:rstrong] (use needinfo to contact me)]

It is upstream code and the fix should be upstreamed first.

Comment 11

[David Volgyes]

(In reply to Emanuel Hoogeveen from comment #7)
> Might be useful to have a tracking bug for all these that could be used e.g.
> to evaluate the usefulness of the tool.

Might be. How can I create a tracking bug you mentioned?

I reported the following bugs with this tool:
673154, 676184, 676179, 676180, 676187, 678785, 678745, 678978, 678982, 678988,678990, 678993, 678997. (Basically, my bug reports are usually based on this tool.)

The Debian project already uses this tool since January. 
See their page: http://qa.debian.org/daca/

Comment 12

[Emanuel Hoogeveen [:ehoogeveen]]

I'm not really the person to ask, but as they're all possible memory leaks - Nicholas, what do you think?

Comment 13

[Nicholas Nethercote [:njn]]

> Nicholas, what do you think?

About having a tracking bug?  Sure, sounds good.  And feel free to put "[MemShrink]" in the whiteboard of any leak-related bug, and also CC me.

BTW, David, if you write "bug XXXXXX" in a comment (i.e. put "bug" in front of the number) Bugzilla will auto-linkify it.

Comment 14

[Emanuel Hoogeveen [:ehoogeveen]]

As for how a tracking bug should look (they're bugs like any other), bug 652780 is probably a good example. They tend to have [meta] or [tracking] in the title, and depend on all the bugs they track.

Comment 15

[:aceman]

As I offered in comment 8, I have created the tracking bug 679417 for you. If you file any new bug, just put 679417 in the "blocks" field of the report. All your bugs will then appear linked to the tracking bug. Thanks for your reports.

Comment 16

[Ed Morley [:emorley]]

David mentioned in bug 679610 comment 4 that he will unlikely have time to follow through with all the patches he has very kindly provided, so if anyone looking at this bug is interested in taking it, please do so - thanks :-)

Comment 17

[Barak Gross]

I'll take it but how do I assign it to myself? sorry, I'm not very familiar with bugzilla :)

A quick look at the 7-zip 9.20 source code showed that that the patch does not apply to it (the CSequentialInStreamRollback class doesn't exist anymore) and I couldn't find any methods that seem related to the one this issue fixed.
I also went over all the allocations in cpp/7zip/common/* and to my eyes all the allocations are matched with deallocations.

To make sure this wasn't a mozilla created class I browsed through the 7zip 4.42 source code (the stable version at 2007-03-22 - the time this was checked into the mozilla branch) and saw that this class did exist there but was subsequently removed in the next version.

So as I understand it upstreaming the fix is no longer relevant, right?

Comment 18

[Ed Morley [:emorley]]

To assign bugs to yourself you need editbugs permissions - and then you press the 'take' link next to the assignee, and press submit. If you don't have editbugs and want someone to change the assignee - just ask in the bug (I've changed this one now for you) :-)

Comment 19

[Barak Gross]

Anyone? I wrote the comment a week ago and no one replied whether what I wrote makes sense or not :)

Also, would it make sense for Mozilla to update to the latest stable 7zip code? I couldn't find any bug relating to it in bugzilla, but looking at http://www.7-zip.org/history.txt shows there have been several iterations of optimizations (though it doesn't specify whether they're decryption or encryption optimizations) in the latest versions... if you think it's a worthy cause I'd be happy to do it :)

Comment 20

[Barak Gross]

Actually, I couldn't find anywhere where anything under other-licenses\7zstub\src was being used...
other-licenses\7zstub\firefox\7zSD.sfx is being used in several firefox makefiles (see http://mxr.mozilla.org/mozilla-central/search?string=7zSD&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=mozilla-central) but I see no references to the src folder.
As a test I renamed the src folder on my local copy and built firefox without a problem, including the installer part (make -C objdir/browser/installer installer) since according to Comment 9 that's where it's used.

So, am I missing something here?

Comment 21

[Ed Morley [:emorley]]

(In reply to Barak Gross from comment #20)
> So, am I missing something here?

The only explanation I can find is in the form of bug 522065 comment 22. Not sure if the src therefore still needs to be kept or not...

Comment 22

[Barak Gross]

Ah ah! so, the source was used initially to create the binary file and was subsequently updated with reshacker (from what I could gather from bugzilla it seems to be because UPX didn't work with the binary built with MSVC8 - see https://bugzilla.mozilla.org/show_bug.cgi?id=348781#c5 and https://bugzilla.mozilla.org/show_bug.cgi?id=368524#c3)
but rebuilding the binary file with the fix should be necessary to fix the leak...
Of course, afterwards we also need to reapply the changes made to the binary file since it was checked in (see http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/other-licenses/7zstub/firefox/7zSD.sfx&rev=HEAD&mark=1.5 and http://hg.mozilla.org/mozilla-central/filelog/da2f5b63ba1e/other-licenses/7zstub/firefox/7zSD.sfx).

As a test I just built the solution (in src/7zip/bundles/SFXSetup-moz) with VS2010, it compiled correctly and UPX seemed to compress the SFX:

...
upx --best -o instgen/7zSD.sfx /c/mozilla-build/mozilla-central/other-licenses/7
zstub/firefox/7zSD.sfx
                       Ultimate Packer for eXecutables
    Copyright (C) 1996,1997,1998,1999,2000,2001,2002,2003,2004,2005,2006
UPX 2.03w       Markus Oberhumer, Laszlo Molnar & John Reiser    Nov 7th 2006

        File size         Ratio      Format      Name
   --------------------   ------   -----------   -----------
    178176 ->     88064   49.43%    win32/pe     7zSD.sfx

Packed 1 file.

So it seems building with MSVC6 isn't necessary anymore, and the easiest solution would be to rebuild with VS2010 after applying to the source all the changes that were previously made to the binary file only... any objections?

A more complex solution would probably be to update to the latest 7zip version (AFAICT all changes made by Mozilla were creating a new bundle in src/7zip/bundles/SFXSetup-moz and removing a lot of unnecessary compression/UI classes from the source) but we can leave that for later ;)

Comment 23

[Barak Gross]

I'm sorry guys, this seems easy enough but I just don't have the time...
I've reset the assignee field to default so this bug is up for grabs.

Comment 24

[:aceman]

Was the 7zip upstream actually contacted?

Comment 25

[Robert Strong [:rstrong] (use needinfo to contact me)]

Since 7-Zip is unlikely to take a patch against such an old version and it appears to be fixed in newer version of 7-Zip I'd prefer we update to the latest 7-Zip vs. fixing the version in our tree... there is other work that is a much higher priority for myself atm though.

Comment 26

[Boris Egorov]

This bug can be closed now, I suppose?

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Rob, I'm not really sure what the status is here. Our in-tree import of the 7-zip code is 4.42. Is there any real value in updating to 9.20?

Comment 28

[Robert Strong [:rstrong] (use needinfo to contact me)]

(In reply to [Away 24-Dec to 4-Jan] Ryan VanderMeulen [:RyanVM UTC-5] from comment #27)
> Rob, I'm not really sure what the status is here. Our in-tree import of the
> 7-zip code is 4.42. Is there any real value in updating to 9.20?
Only to fix this bug.

Comment 29

[Takanori MATSUURA]

Now 15.14 is the latest released version.
http://www.7-zip.org/history.txt

Comment 30

[Takanori MATSUURA]

Have bug 1436475 fixed this?