Community
Participate
Working Groups
Build ID: N/A Steps To Reproduce: 1. Take any bundle and set its file permissions so that the currently logged OS user doesn’t have read permissions to that file. 2. Start Equinox, while still logged as the same OS user 3. Install the bundle by reference 4. Observe the exception. More information: If you're trying to install a bundle by reference when you don't have read permission for that bundle, you get org.eclipse.osgi.service.pluginconversion.PluginConversionException: org.osgi.framework.BundleException: Error converting plugin at D:\osgi_test\BasicMovieFinder.jar. at org.eclipse.core.runtime.internal.adaptor.EclipseStorageHook.generateManifest(EclipseStorageHook.java:411) at org.eclipse.core.runtime.internal.adaptor.EclipseStorageHook.getGeneratedManifest(EclipseStorageHook.java:388) at org.eclipse.core.runtime.internal.adaptor.EclipseStorageHook.createCachedManifest(EclipseStorageHook.java:367) at org.eclipse.core.runtime.internal.adaptor.EclipseStorageHook.getManifest(EclipseStorageHook.java:472) at org.eclipse.osgi.internal.baseadaptor.BaseStorage.loadManifest(BaseStorage.java:297) at org.eclipse.osgi.internal.baseadaptor.BundleInstall.begin(BundleInstall.java:82) at org.eclipse.osgi.framework.internal.core.Framework.installWorkerPrivileged(Framework.java:938) at org.eclipse.osgi.framework.internal.core.Framework$1.run(Framework.java:824) at java.security.AccessController.doPrivileged(Native Method) at org.eclipse.osgi.framework.internal.core.Framework.installWorker(Framework.java:905) at org.eclipse.osgi.framework.internal.core.Framework.installBundle(Framework.java:819) at org.eclipse.osgi.framework.internal.core.BundleContextImpl.installBundle(BundleContextImpl.java:215) at org.eclipse.osgi.framework.internal.core.FrameworkCommandProvider._install(FrameworkCommandProvider.java:310) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25) at java.lang.reflect.Method.invoke(Method.java:585) at org.eclipse.osgi.framework.internal.core.FrameworkCommandInterpreter.execute(FrameworkCommandInterpreter.java:150) at org.eclipse.osgi.framework.internal.core.FrameworkConsole.docommand(FrameworkConsole.java:302) at org.eclipse.osgi.framework.internal.core.FrameworkConsole.console(FrameworkConsole.java:287) at org.eclipse.osgi.framework.internal.core.FrameworkConsole.run(FrameworkConsole.java:223) at java.lang.Thread.run(Thread.java:595) Caused by: org.eclipse.osgi.service.pluginconversion.PluginConversionException: Could not find a META-INF/MANIFEST.MF, plugin.xml or a fragment.xml in D:\osgi_test\BasicMovieFinder.jar. at org.eclipse.core.runtime.internal.adaptor.PluginConverterImpl.fillPluginInfo(PluginConverterImpl.java:107) at org.eclipse.core.runtime.internal.adaptor.PluginConverterImpl.convertManifest(PluginConverterImpl.java:707) at org.eclipse.core.runtime.internal.adaptor.EclipseStorageHook.generateManifest(EclipseStorageHook.java:408) ... 21 more This exception doesn't help to find the real cause of failed installation which is that you can't read the file. If you install the bundle in the normal way(not by reference) you get the right explanation: java.io.FileNotFoundException: D:\osgi_test\BasicMovieFinder.jar (Access is denied). The problem comes from the different behaviour of the connect() method of ReferenceURLConnection and FileURLConnection. FileURLConnection performs "real" connection to the file in its connect() method, so it throws FileNotFoundException if for some reason the file cannot be opened for reading. ReferenceURLConnection connect method throws FileNotFoundException only if the file doesn't exist. So ReferenceURLConnection simply ignores the fact that the file can't be read and installation continues. It fails with PluginConversionException at the next step when trying to read something from the bundle - at this point the bundle is considered as available so the real cause is lost.
Created attachment 139412 [details] Patch in ReferenceURLConnection adding a check if the bundle file may be open The patch adds a check in the ReferenceURLConnection.connect() if the file, from which the bundle will be installed by reference, may be opened. Currently there is only a check if the file exists. The patch substitutes this check with an attempt to open the file.
Thanks, I will investigate for 3.6 M1.
Created attachment 142703 [details] canRead patch This patch uses File.canRead instead of creating a FileInputStream. Using a FileInputStream breaks reference installs for directory bundles because you cannot create an FileInputStream to a directory.
Created attachment 142816 [details] more accurate (access denied) error message This patch does both an exists and canRead check to provide a better error message when the file exists but the user cannot read it.
Fixed in HEAD.
(In reply to comment #4) > Created an attachment (id=142816) [details] > more accurate (access denied) error message > > This patch does both an exists and canRead check to provide a better error > message when the file exists but the user cannot read it. > Hi Tom, It was my mistake that I overlooked the possibility of directory bundles, but I tried opening an InputStream instead of calling canRead, because I think that canRead is not reliable. I made a simple test with a file, to which all access was denied for the current OS user. When calling canRead() for this file, it returned true, but trying to open an InputStream to it resulted in FileNotFound exception with message Access denied. This is under Windows Vista and with jdk1.6.0_10.
It turns out that canRead() does not return correct values on Windows - this is bug 6203387 from sun's bug database. It is opened for java 1.5.0, but as it seems, it is not fixed yet. So probably more complicated checks should be made - if the url is to a file, then trying to open an InputStream to it, if the url is to a directory, then trying to open a stream to a file in the directory?
(In reply to comment #7) > It turns out that canRead() does not return correct values on Windows - this is > bug 6203387 from sun's bug database. It is opened for java 1.5.0, but as it > seems, it is not fixed yet. > So probably more complicated checks should be made - if the url is to a file, > then trying to open an InputStream to it, if the url is to a directory, then > trying to open a stream to a file in the directory? > OK, we will have to check isFile and do the appropriate thing to work around this bug.
Created attachment 143358 [details] patch fixing a bug due to a bug in File.canRead() This patch checks if the bundle file is a file, and if so - tries to open a stream to it to check if it is accessible. If the bundle file is a directory, the fix tries to open the manifest file. If the access to the manifest file is denied, it assumes that the bundle directory is not accessible. If the manifest file is not present - this is not a correct bundle - it tries to list the files in the bundle directory. File.listFiles() returns null in case of IO error during listing, including if read access to the directory is denied.
Created attachment 143399 [details] Fix w/ propagated SecurityException (In reply to comment #9) > Created an attachment (id=143358) [details] > patch fixing a bug due to a bug in File.canRead() > > This patch checks if the bundle file is a file, and if so - tries to open a > stream to it to check if it is accessible. If the bundle file is a directory, > the fix tries to open the manifest file. If the access to the manifest file is > denied, it assumes that the bundle directory is not accessible. If the manifest > file is not present - this is not a correct bundle - it tries to list the files > in the bundle directory. File.listFiles() returns null in case of IO error > during listing, including if read access to the directory is denied. > Nothing is simple anymore. I'm really glad I did not try to fix this in 3.5 ;-) I think there is no need to catch SecurityExceptions and rethrow IOExceptions. We have other places where a SecurityException would get thrown and I think that is the correct behavior. For example, the original call File.exists() method will throw a SecurityException also. Lazar, I simplified the code a bit. Please review and let me know if you have any issues with this patch. Note that this patch is against HEAD not against the 3.5 stream.
(In reply to comment #10) > Created an attachment (id=143399) [details] > Fix w/ propagated SecurityException > > (In reply to comment #9) > > Created an attachment (id=143358) [details] [details] > > patch fixing a bug due to a bug in File.canRead() > > > > This patch checks if the bundle file is a file, and if so - tries to open a > > stream to it to check if it is accessible. If the bundle file is a directory, > > the fix tries to open the manifest file. If the access to the manifest file is > > denied, it assumes that the bundle directory is not accessible. If the manifest > > file is not present - this is not a correct bundle - it tries to list the files > > in the bundle directory. File.listFiles() returns null in case of IO error > > during listing, including if read access to the directory is denied. > > > > Nothing is simple anymore. I'm really glad I did not try to fix this in 3.5 > ;-) > > I think there is no need to catch SecurityExceptions and rethrow IOExceptions. > We have other places where a SecurityException would get thrown and I think > that is the correct behavior. For example, the original call File.exists() > method will throw a SecurityException also. > > Lazar, I simplified the code a bit. Please review and let me know if you have > any issues with this patch. Note that this patch is against HEAD not against > the 3.5 stream. > I checked the patch and it looks better now :) Only one problem - the check manifestFile.isFile() - if the manifest file is not accessible, this will return false even if the file exists and is a file, so we will not try to open a stream to it.
(In reply to comment #11) > I checked the patch and it looks better now :) Only one problem - the check > manifestFile.isFile() - if the manifest file is not accessible, this will > return false even if the file exists and is a file, so we will not try to open > a stream to it. > But would this not cause an issue for the orginal file.isFile() check we do? Is it possible that file.isFile() and file.isDirectory() will both return false?
Created attachment 143402 [details] Modified fix I avoid the file.isFile call. If isDirectory returns false I am going to assume it is a file. This may not work in all cases. For example, I am not sure what how linked files are handled on unix style file systems.
(In reply to comment #13) > Created an attachment (id=143402) [details] > Modified fix > > I avoid the file.isFile call. If isDirectory returns false I am going to > assume it is a file. This may not work in all cases. For example, I am not > sure what how linked files are handled on unix style file systems. > The problem is that when a file is placed in a directory, to which we have no read access, no checks for this file can be made - exists(), isFile() and isDirectory() return false (probably since we have no access to the directory, we cannot check the attributes of the files in it). So we cannot count on them. On the other hand, if we have a file (directory) with no read permissions, but it is in a directory, for which we have read permissions, the above methods return the correct value for this file (directory). So the original checks are ok as long as the bundle file/directory is located in a directory, to which we have read access. The problem is when we make checks for the manifest file, and we have no read permissions for the bundle directory. Probably in this case we should not make the above checks, but instead try to open the stream in a try/catch block and check in the exception message - if there is an exception. If the manifest file does not exist, we will have FileNotFoundException, while if the file exists, we will have the same exception, but with (Access is denied) in the message. Here I assume that the manifest either does not exist, or exists and is a file.
Created attachment 143510 [details] Modified fix 2 (In reply to comment #14) > The problem is when we make checks for the manifest file, and we have no read > permissions for the bundle directory. Probably in this case we should not make > the above checks, but instead try to open the stream in a try/catch block and > check in the exception message - if there is an exception. If the manifest file > does not exist, we will have FileNotFoundException, while if the file exists, > we will have the same exception, but with (Access is denied) in the message. > Here I assume that the manifest either does not exist, or exists and is a file. > I'm not keen on checking the return message for (Access is denied). I think we should just do a simple listFiles check in the directory case. I realize this may not be the most complete fix but I think it is the safest thing to do at this point. I did some testing with soft links on the Mac and the code works correctly in that case. I was concerned about how isFile and isDirectory would behave. It appears they consult the file that they are soft linked to which is what I would expect.
(In reply to comment #15) > > I'm not keen on checking the return message for (Access is denied). I think we > should just do a simple listFiles check in the directory case. I realize this > may not be the most complete fix but I think it is the safest thing to do at > this point. > Yes, I agree with that.
Latest patch released to head.
