Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 349121

Summary:

SignatureBlockProcessor.readIntoArray(BundleEntry) does not close input stream

Product:

[Eclipse Project] Equinox

Reporter:

Pascal Rapicault <pascal>

Component:

Framework

Assignee:

Thomas Watson <tjwatson>

Status:

RESOLVED FIXED

QA Contact:

Severity:

critical

Priority:

CC:

david_williams, mknauer, stephan.herrmann

Version:

3.7

Target Milestone:

Juno M1

Hardware:

OS:

Mac OS X - Carbon (unsup.)

Whiteboard:

Bug Depends on:

Bug Blocks:

349192

Attachments:

Description	Flags
patch	none
updated patch	none

Description Pascal Rapicault

2011-06-11 10:54:15 EDT

From bug #300589, Stephan found the following:

OK, since this bug is still relevant, I had a quick look at the source of

org.eclipse.osgi.internal.signedcontent.SignatureBlockProcessor.readIntoArray(BundleEntry)

It looks like a very simply file handle leak here: the InputStream
"is" might be allocated in be.getInputStream() but is never closed.

Looks like a simple
  } finally {
     if (is != null) is.close();
  }
might solve the issue at this location (there might be more!!).

I'm pretty confident that this helps because I previously had the
same bug pattern in my code and the finally did fix one instance
of "Too many open files" :)


Then in a second comments he adds:
More evidence: the error message in bug 349073 reads
"Error reading signed content". That's logged from
org.eclipse.equinox.internal.p2.artifact.repository.SignatureVerifier.verifyContent()

When drilling into verifierFactory.getSignedContent(inputFile);
we actually get to the same readIntoArray() method mentioned in my
previous comment.

Comment 1 Thomas Watson

2011-06-13 09:46:04 EDT

Created attachment 197885 [details]
patch

Comment 2 Thomas Watson

2011-06-13 09:47:45 EDT

Thanks for the find Stephan!  I released a fix to HEAD.  I will open a separate bug to fix for Indigo SR1.

Comment 3 Thomas Watson

2011-06-13 09:49:21 EDT

fixed.

Comment 4 Stephan Herrmann

2011-06-13 09:59:54 EDT

(In reply to comment #2)
> Thanks for the find Stephan!

welcome :)

>  I released a fix to HEAD. 

Regarding your patch: Are you sure "is" will *never* be null?
Would be a pity if the method throws just during cleanup, no?
I seem to see implementors of BundleEntry.getInputStream()
that can indeed return null.
Also: how about IOException during close?
Just wondering..

Comment 5 Thomas Watson

2011-06-13 10:19:22 EDT

Created attachment 197886 [details]
updated patch

(In reply to comment #4)
> (In reply to comment #2)
> > Thanks for the find Stephan!
> 
> welcome :)
> 
> >  I released a fix to HEAD. 
> 
> Regarding your patch: Are you sure "is" will *never* be null?

BundleEntry object should never return null here.  An IOException must be thrown if the entry cannot produce an InputStream.

> Would be a pity if the method throws just during cleanup, no?
> I seem to see implementors of BundleEntry.getInputStream()
> that can indeed return null.

Which implementors?  I found DirZipBundleEntry returned null, incorrectly and is fixed in this patch.  Are there others?

> Also: how about IOException during close?
> Just wondering..

Here is an update patch that catches the exception on close.

Comment 6 Stephan Herrmann

2011-06-13 11:33:11 EDT

(In reply to comment #5)
> > Regarding your patch: Are you sure "is" will *never* be null?
> 
> BundleEntry object should never return null here.  An IOException must be
> thrown if the entry cannot produce an InputStream.

Great, so we know what *should* happen.

> > Would be a pity if the method throws just during cleanup, no?
> > I seem to see implementors of BundleEntry.getInputStream()
> > that can indeed return null.
> 
> Which implementors?  I found DirZipBundleEntry returned null, incorrectly and
> is fixed in this patch.  Are there others?

Actually ZipBundleEntry returns ZipFile.getInputStream() which is
"internally documented" to return null for non-existing entries 
(when looking at JDK < 1.7: via the private getInputStream(String))

Comment 7 Thomas Watson

2011-06-13 11:50:26 EDT

(In reply to comment #6)
> 
> Actually ZipBundleEntry returns ZipFile.getInputStream() which is
> "internally documented" to return null for non-existing entries 
> (when looking at JDK < 1.7: via the private getInputStream(String))

This should not happen since we only construct ZipBundleEntry objects after confirming that the entry actually exists in the ZipFile.

Comment 8 Thomas Watson

2011-06-13 12:08:27 EDT

FYI, the latest patch was released.  I am reluctant to do any null checks for inputstreams here since we make the assumption all over in the framework that null is never returned here.