Bug 102434 - Trying to create ELF archive for non-archive binary files causes problems (patch included)
Summary: Trying to create ELF archive for non-archive binary files causes problems (pa...
Status: RESOLVED FIXED
Alias: None
Product: CDT
Classification: Tools
Component: cdt-core (show other bugs)
Version: 3.0   Edit
Hardware: PC Linux
: P3 normal (vote)
Target Milestone: 3.0 RC2   Edit
Assignee: Alain Magloire CLA
QA Contact:
URL:
Whiteboard:
Keywords: contributed
Depends on:
Blocks:
 
Reported: 2005-06-30 22:15 EDT by Robert O'Callahan CLA
Modified: 2008-06-19 13:02 EDT (History)
3 users (show)

See Also:


Attachments
fix (6.39 KB, patch)
2005-06-30 22:16 EDT, Robert O'Callahan CLA
bjorn.freeman-benson: iplog+
Details | Diff
fix to detect the mach-o header correctly (2.80 KB, patch)
2005-07-07 22:34 EDT, Chris Wiebe CLA
bjorn.freeman-benson: iplog+
Details | Diff
patch to ensure the inputstream gets closed (1.23 KB, patch)
2005-07-08 13:50 EDT, Chris Wiebe CLA
bjorn.freeman-benson: iplog+
Details | Diff
Read magic bytes from the candidate archive file in constructor of AR (1.82 KB, patch)
2008-01-31 02:16 EST, Abeer Bagul CLA
bjorn.freeman-benson: iplog+
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robert O'Callahan CLA 2005-06-30 22:15:56 EDT
When CModelManager sees a binary file it can't identify, it calls
IBinaryParser.getBinary for each binary parser available. ElfParser sees that
it's not an ELF file and calls createBinaryArchive on it, which eventually
constructs an AR object to see if the file exists. The AR object examines the
file by opening it and calling readLine(); if it's not a real ar file, then it
throws an exception which is caught by CModelManager. Unfortunately if you have
a very large non-ELF binary file in your project directories which happens to
not contain newlines, this causes massive memory allocation and I/O when we only
really needed to look at the first few bytes.

I have a patch that fixes this problem. It makes CModelManager call
parser.isBinary before we actually do a getBinary. To get that to work
correctly, I had to fix an issue in the code that reads the file header hints;
we must ensure that the number of bytes returned in the hints array is no less
than the number of bytes available in the file. Currently it just calls
readBytes once, which may return less 'hints' bytes even if there is more data
available in the file. The patch also removes isBinary calls that are now redundant.
Comment 1 Robert O'Callahan CLA 2005-06-30 22:16:22 EDT
Created attachment 24249 [details]
fix

patch as described
Comment 2 John Camelon CLA 2005-07-04 23:39:50 EDT
This would be a good fix for 3.0.  
Comment 3 David Inglis CLA 2005-07-05 15:54:33 EDT
patch applied
Comment 4 Chris Wiebe CLA 2005-07-07 21:29:29 EDT
Unfortunately this patch totally breaks the MachO parser.  I don't get any binaries showing up at all.

If I revert CModelManager back to revision 1.95 it starts working again.
Comment 5 Chris Wiebe CLA 2005-07-07 22:34:32 EDT
Created attachment 24453 [details]
fix to detect the mach-o header correctly

This should fix the problem.  I don't know how this could have ever worked -
must be exercising this code path for the first time. :)
Comment 6 Chris Wiebe CLA 2005-07-08 13:50:42 EDT
Created attachment 24484 [details]
patch to ensure the inputstream gets closed

make sure the inputstream gets closed if an exception is thrown
Comment 7 Alain Magloire CLA 2005-07-08 17:35:33 EDT
Patches are in,
Please verify.
Comment 8 Abeer Bagul CLA 2008-01-31 02:13:24 EST
Part 2 of the same problem is as follows:

The ElfParser has two methods to create an IBinaryFile. 
1) public IBinaryFile getBinary(IPath path) throws IOException
2) public IBinaryFile getBinary(byte[] hints, IPath path) throws IOException

The second method is called from CModelManager. This flow checks that the file is indeed a binary or archive by reading the hint bytes.

However, the flow which calls the method 1 does not read hint bytes from the candidate file. Hence, the problem which is already solved for the flow in the method 2 again occurs for method 1. 

To fix this, I have contributed a patch which reads hint bytes from the candidate file in the constructor of AR. 
The number of bytes read is not the same as the hint number indicated by the ElfParser (i.e. 128 bytes) but just enough to cover the AR magic string "!<arch>" (i.e. 7 bytes).

-------------------

Also, the patch fixes the method:
private String removeBlanks(String str)
from class ARHeader.

The problem encountered here was unsafe decoding of int. removeblanks(...) was only removing trailing spaces. The method now uses trim() which does a better job.
Comment 9 Abeer Bagul CLA 2008-01-31 02:16:26 EST
Created attachment 88376 [details]
Read magic bytes from the candidate archive file in constructor of AR
Comment 10 Anton Leherbauer CLA 2008-02-01 10:26:18 EST
(In reply to comment #9)
> Created an attachment (id=88376) [details]
> Read magic bytes from the candidate archive file in constructor of AR

Thanks for the patch. I have fixed the issue re-using parts of your patch together with some performance improvements. Please verify.