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)

Product:
Firefox
Component:
General
Platform:
x86
Windows 7
Type:
defect

Tracking

()

People

(Reporter: david.volgyes, Unassigned)

References

Details

(Keywords: memory-leak)

Attachments

(1 obsolete file)

Attached patch proposed fix. (obsolete) — — Splinter Review
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[]
Keywords: mlk
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 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?
Yes, they are scattered hither and yon.
(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.
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?
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.
It is upstream code and the fix should be upstreamed first.
(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/
I'm not really the person to ask, but as they're all possible memory leaks - Nicholas, what do you think?
> 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.
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.
Blocks: cppcheck
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.
Whiteboard: patch → [needs upstreaming first]
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]
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?
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
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 :)
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?
(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...
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 ;)
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
Was the 7zip upstream actually contacted?
Status: ASSIGNED → NEW
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
This bug can be closed now, I suppose?
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]
Attachment #550296 - Attachment is obsolete: true
(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)
Now 15.14 is the latest released version. http://www.7-zip.org/history.txt
Have bug 1436475 fixed this?
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: