[cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Hi all,
Opening a java.io.RandomAccessFile in read-only mode with a security
manager in force requires the permission to write file descriptors.
The attached patch fixes. Anyone mind if I commit?
Cheers,
Gary
Index: ChangeLog
===
RCS file: /cvsroot/classpath/classpath/ChangeLog,v
retrieving revision 1.5764
diff -u -r1.5764 ChangeLog
--- ChangeLog 5 Dec 2005 13:25:00 - 1.5764
+++ ChangeLog 5 Dec 2005 14:36:19 -
@@ -1,3 +1,9 @@
+2005-11-18 Gary Benson <[EMAIL PROTECTED]>
+
+ * java/io/RandomAccessFile.java (RandomAccessFile): Don't create
+ DataOutputStream for read-only files to avoid unnecessary security
+ manager check.
+
2005-12-05 Mark Wielaard <[EMAIL PROTECTED]>
Fixes bug classpath/25257
Index: java/io/RandomAccessFile.java
===
RCS file: /cvsroot/classpath/classpath/java/io/RandomAccessFile.java,v
retrieving revision 1.47
diff -u -r1.47 RandomAccessFile.java
--- java/io/RandomAccessFile.java 2 Jul 2005 20:32:38 - 1.47
+++ java/io/RandomAccessFile.java 5 Dec 2005 14:36:19 -
@@ -124,7 +124,10 @@
ch = FileChannelImpl.create(file, fdmode);
fd = new FileDescriptor(ch);
-out = new DataOutputStream (new FileOutputStream (fd));
+if ((fdmode & FileChannelImpl.WRITE) != 0)
+ out = new DataOutputStream (new FileOutputStream (fd));
+else
+ out = null;
in = new DataInputStream (new FileInputStream (fd));
}
@@ -766,6 +769,9 @@
*/
public void write (int oneByte) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.write(oneByte);
}
@@ -777,6 +783,9 @@
*/
public void write (byte[] buffer) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.write(buffer);
}
@@ -792,6 +801,9 @@
*/
public void write (byte[] buffer, int offset, int len) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.write (buffer, offset, len);
}
@@ -806,6 +818,9 @@
*/
public final void writeBoolean (boolean val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeBoolean(val);
}
@@ -820,6 +835,9 @@
*/
public final void writeByte (int val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeByte(val);
}
@@ -834,6 +852,9 @@
*/
public final void writeShort (int val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeShort(val);
}
@@ -848,6 +869,9 @@
*/
public final void writeChar (int val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeChar(val);
}
@@ -861,6 +885,9 @@
*/
public final void writeInt (int val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeInt(val);
}
@@ -874,6 +901,9 @@
*/
public final void writeLong (long val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeLong(val);
}
@@ -893,6 +923,9 @@
*/
public final void writeFloat (float val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeFloat(val);
}
@@ -913,6 +946,9 @@
*/
public final void writeDouble (double val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeDouble(val);
}
@@ -927,6 +963,9 @@
*/
public final void writeBytes (String val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeBytes(val);
}
@@ -941,6 +980,9 @@
*/
public final void writeChars (String val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeChars(val);
}
@@ -975,6 +1017,9 @@
*/
public final void writeUTF (String val) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
out.writeUTF(val);
}
___
Classpath-patches mailing list
Classpath-patches@gnu.org
http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Gary Benson wrote:
Hi all,
Opening a java.io.RandomAccessFile in read-only mode with a security
manager in force requires the permission to write file descriptors.
The attached patch fixes. Anyone mind if I commit?
Cheers,
Gary
Index: ChangeLog
===
RCS file: /cvsroot/classpath/classpath/ChangeLog,v
retrieving revision 1.5764
diff -u -r1.5764 ChangeLog
--- ChangeLog 5 Dec 2005 13:25:00 - 1.5764
+++ ChangeLog 5 Dec 2005 14:36:19 -
@@ -1,3 +1,9 @@
+2005-11-18 Gary Benson <[EMAIL PROTECTED]>
+
+ * java/io/RandomAccessFile.java (RandomAccessFile): Don't create
+ DataOutputStream for read-only files to avoid unnecessary security
+ manager check.
+
2005-12-05 Mark Wielaard <[EMAIL PROTECTED]>
Fixes bug classpath/25257
Index: java/io/RandomAccessFile.java
===
RCS file: /cvsroot/classpath/classpath/java/io/RandomAccessFile.java,v
retrieving revision 1.47
diff -u -r1.47 RandomAccessFile.java
--- java/io/RandomAccessFile.java 2 Jul 2005 20:32:38 - 1.47
+++ java/io/RandomAccessFile.java 5 Dec 2005 14:36:19 -
@@ -124,7 +124,10 @@
ch = FileChannelImpl.create(file, fdmode);
fd = new FileDescriptor(ch);
-out = new DataOutputStream (new FileOutputStream (fd));
+if ((fdmode & FileChannelImpl.WRITE) != 0)
+ out = new DataOutputStream (new FileOutputStream (fd));
+else
+ out = null;
in = new DataInputStream (new FileInputStream (fd));
}
@@ -766,6 +769,9 @@
*/
public void write (int oneByte) throws IOException
{
+if (out == null)
+ throw new IOException("Bad file descriptor");
+
The real error is that the file was opened read-only. A message saying
"Bad file descriptor" does not convey that information. If the only
reason that out can be null is that it was opened read-only, then I
think all the exception messages should reflect that.
David Daney.
___
Classpath-patches mailing list
Classpath-patches@gnu.org
http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
On Mon, 2005-12-05 at 14:42 +, Gary Benson wrote:
>public void write (int oneByte) throws IOException
>{
> +if (out == null)
> + throw new IOException("Bad file descriptor");
> +
> out.write(oneByte);
I don't know if this is performance critical code or is used very often,
but this seems to be a special case and i'd suggest something like:
public void write (int oneByte) throws IOException
{
try {
out.write(oneByte);
return;
} catch (NullPointerException e) {
throw new IOException("Bad file descriptor");
}
}
I'll not win a beautiful-code prize, but will be compiled to the fastest
code.
TWISTI
___
Classpath-patches mailing list
Classpath-patches@gnu.org
http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Christian Thalinger wrote:
I don't know if this is performance critical code or is used very often,
but this seems to be a special case and i'd suggest something like:
public void write (int oneByte) throws IOException
{
try {
out.write(oneByte);
return;
} catch (NullPointerException e) {
throw new IOException("Bad file descriptor");
}
}
I'll not win a beautiful-code prize, but will be compiled to the fastest
code.
That's getting into the micro-optimzation realm, which is
fraught with danger and mistaken assumptions :-) E.g., on
some machines the time overhead of setting up a try/catch in
a method that wouldn't otherwise have one is higher than
the single comparison required to test for null. In particular,
any interpreter is going to have to test for null anyway,
so the second time it's already in cache, blah blah, etc.
Not to mention that the space overhead of a try/catch (vs. none)
will probably be higher too. So IMHO it's better to avoid
too much of this kind of thing (unless it actually makes the
source code clearer (don't think so in this case)).
Apologies for the minor IMHO rant.. :-)
-Archie
__
Archie Cobbs *CTO, Awarix* http://www.awarix.com
___
Classpath-patches mailing list
Classpath-patches@gnu.org
http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
On Tue, 2005-12-06 at 10:00 -0600, Archie Cobbs wrote: > That's getting into the micro-optimzation realm, which is > fraught with danger and mistaken assumptions :-) E.g., on > some machines the time overhead of setting up a try/catch in > a method that wouldn't otherwise have one is higher than > the single comparison required to test for null. In particular, > any interpreter is going to have to test for null anyway, > so the second time it's already in cache, blah blah, etc. "...setting up a try/catch..."? What do you mean? Agreed on the interpreter thing, but who does benchmarks on interpreters? > > Not to mention that the space overhead of a try/catch (vs. none) > will probably be higher too. So IMHO it's better to avoid > too much of this kind of thing (unless it actually makes the > source code clearer (don't think so in this case)). Actually the generated code is smaller and i don't think there is too much space overhead in the VM (at least not in CACAO). Yes, the code is not clearer but it's just a 6-liner in a core class... so it should be understandable for everyone :-) Whatever... /me still trying to kick sun's ass TWISTI ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Christian Thalinger wrote: That's getting into the micro-optimzation realm, which is fraught with danger and mistaken assumptions :-) E.g., on some machines the time overhead of setting up a try/catch in a method that wouldn't otherwise have one is higher than the single comparison required to test for null. In particular, any interpreter is going to have to test for null anyway, so the second time it's already in cache, blah blah, etc. "...setting up a try/catch..."? What do you mean? Agreed on the Simply that on some VM's there is overhead associated with executing code that can possibly catch an exception, even if no exception is actually thrown. For example, locals might be stored on the stack instead of in registers if the engine doesn't know how to unwind registers. Not to mention that the space overhead of a try/catch (vs. none) will probably be higher too. So IMHO it's better to avoid too much of this kind of thing (unless it actually makes the source code clearer (don't think so in this case)). Actually the generated code is smaller and i don't think there is too much space overhead in the VM (at least not in CACAO). Yes, the code is not clearer but it's just a 6-liner in a core class... so it should be understandable for everyone :-) You say "the generated code is smaller" but that depends on who generates the code (if you mean the generated Java bytecode, I'd guess you're wrong there too because of the extra exception table required). And that depends on which VM you're using.. which is my only point: it all depends, so you can't assume anything for sure. I didn't really mean to criticize this change so much as the idea of initiating a wild goose chase of similar changes which may or may not actually improve anything. -Archie __ Archie Cobbs *CTO, Awarix* http://www.awarix.com ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Christian Thalinger wrote: On Tue, 2005-12-06 at 10:00 -0600, Archie Cobbs wrote: That's getting into the micro-optimzation realm, which is fraught with danger and mistaken assumptions :-) E.g., on some machines the time overhead of setting up a try/catch in a method that wouldn't otherwise have one is higher than the single comparison required to test for null. In particular, any interpreter is going to have to test for null anyway, so the second time it's already in cache, blah blah, etc. "...setting up a try/catch..."? What do you mean? Agreed on the interpreter thing, but who does benchmarks on interpreters? On gcj using SJLJ exceptions there is overhead for entering a try block. And for the dwarf unwinder there is overhead in the unwinding tables (but not in the code) for each try/catch. That you don't care about the implications for platforms other than your own, does not imply that they do not exist. David Daney ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
On Tue, 2005-12-06 at 13:32 -0600, Archie Cobbs wrote: > You say "the generated code is smaller" but that depends on who generates > the code (if you mean the generated Java bytecode, I'd guess you're wrong > there too because of the extra exception table required). And that depends > on which VM you're using.. which is my only point: it all depends, so you > can't assume anything for sure. > > I didn't really mean to criticize this change so much as the idea of > initiating a wild goose chase of similar changes which may or may not > actually improve anything. Yeah, i didn't take it personally :-) Of course i see your point, but what i'm trying to say is, if we ever want to catch up (or even be better) than sun or other proprietary JVMs, we should think about optimizing some "core" functions in classpath... TWISTI ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Christian Thalinger wrote: what i'm trying to say is, if we ever want to catch up (or even be better) than sun or other proprietary JVMs, we should think about optimizing some "core" functions in classpath... I definitely agree there. -Archie __ Archie Cobbs *CTO, Awarix* http://www.awarix.com ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
> "Twisti" == Christian Thalinger <[EMAIL PROTECTED]> writes: Twisti> Yeah, i didn't take it personally :-) Of course i see your point, but Twisti> what i'm trying to say is, if we ever want to catch up (or even be Twisti> better) than sun or other proprietary JVMs, we should think about Twisti> optimizing some "core" functions in classpath... Yeah. This is tricky for us since the various VMs differ. That said, I think we've seen a number of performance fixes go in during the last year, and for the most part these haven't been micro-optimizations. In this particular case, I think RandomAccessFile is not used very much. I would only bother looking at optimizations in this code if it showed up in a profile of some application. Tom ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Tom Tromey wrote: > > "Twisti" == Christian Thalinger <[EMAIL PROTECTED]> writes: > Twisti> Yeah, i didn't take it personally :-) Of course i see your > Twisti> point, but what i'm trying to say is, if we ever want to > Twisti> catch up (or even be better) than sun or other proprietary > Twisti> JVMs, we should think about optimizing some "core" functions > Twisti> in classpath... > > Yeah. This is tricky for us since the various VMs differ. > > That said, I think we've seen a number of performance fixes go in > during the last year, and for the most part these haven't been > micro-optimizations. > > In this particular case, I think RandomAccessFile is not used very > much. I would only bother looking at optimizations in this code if > it showed up in a profile of some application. Ok, so I'll commit my original patch for now. Cheers, Gary ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Gary Benson wrote: Tom Tromey wrote: "Twisti" == Christian Thalinger <[EMAIL PROTECTED]> writes: Twisti> Yeah, i didn't take it personally :-) Of course i see your Twisti> point, but what i'm trying to say is, if we ever want to Twisti> catch up (or even be better) than sun or other proprietary Twisti> JVMs, we should think about optimizing some "core" functions Twisti> in classpath... Yeah. This is tricky for us since the various VMs differ. That said, I think we've seen a number of performance fixes go in during the last year, and for the most part these haven't been micro-optimizations. In this particular case, I think RandomAccessFile is not used very much. I would only bother looking at optimizations in this code if it showed up in a profile of some application. Ok, so I'll commit my original patch for now. I hate to sound like I have a burr under the saddle, but does anybody see any merit whatsoever in changing the exception text as I suggested in my previous response to the patch? David Daney. ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
David Daney wrote: > Gary Benson wrote: > > ...I'll commit my original patch for now. > > I hate to sound like I have a burr under the saddle, but does > anybody see any merit whatsoever in changing the exception text > as I suggested in my previous response to the patch? What did you suggest? I saw your mail about exception handling, but that's all... Cheers, Gary ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
Gary Benson wrote: David Daney wrote: Gary Benson wrote: ...I'll commit my original patch for now. I hate to sound like I have a burr under the saddle, but does anybody see any merit whatsoever in changing the exception text as I suggested in my previous response to the patch? What did you suggest? I saw your mail about exception handling, but that's all... http://lists.gnu.org/archive/html/classpath-patches/2005-12/msg00042.html David Daney ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
> "David" == David Daney <[EMAIL PROTECTED]> writes: David> I hate to sound like I have a burr under the saddle, but does anybody David> see any merit whatsoever in changing the exception text as I suggested David> in my previous response to the patch? Yes, I was in favor of this as well. Tom ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
Re: [cp-patches] Patch: Opening RandomAccessFiles requires excessive permission
David Daney wrote: > Gary Benson wrote: > > David Daney wrote: > > > Gary Benson wrote: > > > > ...I'll commit my original patch for now. > > > > > > I hate to sound like I have a burr under the saddle, but does > > > anybody see any merit whatsoever in changing the exception text > > > as I suggested in my previous response to the patch? > > > > What did you suggest? I saw your mail about exception handling, > > but that's all... > > http://lists.gnu.org/archive/html/classpath-patches/2005-12/msg00042.html Wierd, I never got that mail. But I don't mind the message changing. How do people feel about "file not opened for writing"? Cheers, Gary ___ Classpath-patches mailing list Classpath-patches@gnu.org http://lists.gnu.org/mailman/listinfo/classpath-patches
