Open
Bug 676179
Opened 14 years ago
Updated 3 years ago
memory allocation/deallocation mismatch in other-licenses/7zstub/src/7zip/Common/StreamObjects.h
Categories
(Firefox :: General, defect)
Tracking
()
NEW
People
(Reporter: david.volgyes, Unassigned)
References
Details
(Keywords: memory-leak)
Attachments
(1 obsolete file)
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•14 years ago
|
||
Taras, I feel like you're one of the few people who has any stake in this code.
Hi, the patch seems logical, have you confirmed it can cause a leak? The keywords are set indicating that.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: patch
Comment 3•14 years ago
|
||
Comment on attachment 550296 [details] [diff] [review]
proposed fix.
nice
Attachment #550296 -
Flags: review+
(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•14 years ago
|
||
Yes, they are scattered hither and yon.
Reporter | ||
Comment 6•14 years ago
|
||
(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•14 years ago
|
||
Might be useful to have a tracking bug for all these that could be used e.g. to evaluate the usefulness of the tool.
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•14 years ago
|
||
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•14 years ago
|
||
It is upstream code and the fix should be upstreamed first.
Reporter | ||
Comment 11•14 years ago
|
||
(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•14 years ago
|
||
I'm not really the person to ask, but as they're all possible memory leaks - Nicholas, what do you think?
![]() |
||
Comment 13•14 years ago
|
||
> 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•14 years ago
|
||
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•14 years ago
|
||
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.
Updated•14 years ago
|
Whiteboard: patch → [needs upstreaming first]
Comment 16•14 years ago
|
||
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 :-)
Whiteboard: [needs upstreaming first] → [needs upstreaming first] [has patch, needs new assignee]
Comment 17•14 years ago
|
||
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•14 years ago
|
||
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) :-)
Assignee: nobody → smoohta
Status: NEW → ASSIGNED
Comment 19•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee: smoohta → nobody
![]() |
||
Comment 25•14 years ago
|
||
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.
OS: Linux → Windows 7
Hardware: x86_64 → x86
Comment 26•11 years ago
|
||
This bug can be closed now, I suppose?
Comment 27•10 years ago
|
||
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?
Flags: needinfo?(robert.strong.bugs)
Whiteboard: [needs upstreaming first] [has patch, needs new assignee]
Updated•10 years ago
|
Attachment #550296 -
Attachment is obsolete: true
![]() |
||
Comment 28•10 years ago
|
||
(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.
Flags: needinfo?(robert.strong.bugs)
Comment 29•9 years ago
|
||
Now 15.14 is the latest released version.
http://www.7-zip.org/history.txt
Comment 30•7 years ago
|
||
Have bug 1436475 fixed this?
Updated•3 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•