Reviewer needed - fix for regression test LastErrorString
Hi all, I'd like to push fix for regression test java/io/IOException/LastErrorString.java to OpenJDK7 and OpenJDK6. This fix ensures that no system-important files can be erased and/or rewritten by this test even if root started JTreg. Fixed test also tries all various combinations of file attributes. Webrev is available at: http://cr.openjdk.java.net/~ptisnovs/jtreg-jdk-test-LastErrorString-fix/ Can anybody please review it? Thank you in advance, Pavel Tisnovsky
Pavel Tisnovsky wrote:
Hi all,
I'd like to push fix for regression test java/io/IOException/LastErrorString.java to OpenJDK7 and OpenJDK6. This fix ensures that no system-important files can be erased and/or rewritten by this test even if root started JTreg. Fixed test also tries all various combinations of file attributes.
Webrev is available at: http://cr.openjdk.java.net/~ptisnovs/jtreg-jdk-test-LastErrorString-fix/
Can anybody please review it?
Thank you in advance, Pavel Tisnovsky
I agree that this test should be fixed but I don't know about jdk7 as it's being stabilized now and only critical changes are allowed. From a quick glance it looks like you've added the @test tag to both files so I assume jtreg will want to run it twice. Have you tried the test on Windows? I assume it will at least fail with "Unrecognized system" if run with Cygwin. In general it's best to avoid scripts if you can - in this case have you considered created an unreadable and unwriteable file in java instead? -Alan.
On 5 May 2011 20:45, Alan Bateman <Alan.Bateman@oracle.com> wrote:
Pavel Tisnovsky wrote:
Hi all,
I'd like to push fix for regression test java/io/IOException/LastErrorString.java to OpenJDK7 and OpenJDK6. This fix ensures that no system-important files can be erased and/or rewritten by this test even if root started JTreg. Fixed test also tries all various combinations of file attributes.
Webrev is available at: http://cr.openjdk.java.net/~ptisnovs/jtreg-jdk-test-LastErrorString-fix/
Can anybody please review it?
Thank you in advance, Pavel Tisnovsky
I agree that this test should be fixed but I don't know about jdk7 as it's being stabilized now and only critical changes are allowed.
Pavel submitted the patch for OpenJDK7, not jdk7.
From a quick glance it looks like you've added the @test tag to both files so I assume jtreg will want to run it twice. Have you tried the test on Windows? I assume it will at least fail with "Unrecognized system" if run with Cygwin. In general it's best to avoid scripts if you can - in this case have you considered created an unreadable and unwriteable file in java instead?
We don't build on Windows.
-Alan.
-- Andrew :-) Support Free Java! Contribute to GNU Classpath and IcedTea http://www.gnu.org/software/classpath http://icedtea.classpath.org PGP Key: F5862A37 (https://keys.indymedia.org/) Fingerprint = EA30 D855 D50F 90CD F54D 0698 0713 C3ED F586 2A37
Alan Bateman wrote:
Pavel Tisnovsky wrote:
Hi all,
I'd like to push fix for regression test java/io/IOException/LastErrorString.java to OpenJDK7 and OpenJDK6. This fix ensures that no system-important files can be erased and/or rewritten by this test even if root started JTreg. Fixed test also tries all various combinations of file attributes.
Webrev is available at: http://cr.openjdk.java.net/~ptisnovs/jtreg-jdk-test-LastErrorString-fix/
Can anybody please review it?
Thank you in advance, Pavel Tisnovsky
Hi Alan,
I agree that this test should be fixed but I don't know about jdk7 as it's being stabilized now and only critical changes are allowed.
just to be sure - are you talking about proprietary JDK7 or OpenJDK7?
From a quick glance it looks like you've added the @test tag to both files so I assume jtreg will want to run it twice.
JTreg harness only "see" LastErrorString.java as test, but you are right, I'll remove unecessary tags from shell script.
Have you tried the test on Windows? I assume it will at least fail with "Unrecognized system" if run with Cygwin. In general it's best to avoid scripts if you can - in this case have you considered created an unreadable and unwriteable file in java instead?
No I did not try to run it on Windows, I just need to fix the possibility of changing/erasing important system files ATM. Will try to create Java-only solution. Thanks Pavel
-Alan.
Pavel Tisnovsky wrote:
:
I agree that this test should be fixed but I don't know about jdk7 as it's being stabilized now and only critical changes are allowed.
just to be sure - are you talking about proprietary JDK7 or OpenJDK7?
I'm talking about the jdk7/jdk7 forest. That's going to get locked down soon and only critical changes will be allowed in. I honestly do not know any of the details as to how this will happen but I don't think it's unreasonable to limit the number of changes when nearing the end. It would be nice if test-only changes could be exempt from red tape but I just don't know at this point. This shouldn't stop us reviewing the change of course and I'm happy to run it on Windows for you. I also need to create a bug for this as I don't think we have one already (FWIW, this test seems to have been added in 1998 and has been largely untouched since then). -Alan.
Alan Bateman wrote:
Pavel Tisnovsky wrote:
:
I agree that this test should be fixed but I don't know about jdk7 as it's being stabilized now and only critical changes are allowed.
just to be sure - are you talking about proprietary JDK7 or OpenJDK7?
I'm talking about the jdk7/jdk7 forest. That's going to get locked down soon and only critical changes will be allowed in. I honestly do not know any of the details as to how this will happen but I don't think it's unreasonable to limit the number of changes when nearing the end.
I see, thanks for the clarification.
It would be nice if test-only changes could be exempt from red tape but I just don't know at this point. This shouldn't stop us reviewing the change of course and I'm happy to run it on Windows for you. I also need to create a bug for this as I don't think we have one already (FWIW, this test seems to have been added in 1998 and has been largely untouched since then).
-Alan.
Alan Bateman wrote:
I also need to create a bug for this as I don't think we have one already Here it is:
7042603: TEST_BUG: test/java/io/IOException/LastErrorString.java touches system files
Alan Bateman wrote:
Alan Bateman wrote:
I also need to create a bug for this as I don't think we have one already Here it is:
7042603: TEST_BUG: test/java/io/IOException/LastErrorString.java touches system files
Hi Alan, thank you for creating bug# for this issue. I changed this regression test so it does not need to use shell script at all because now it creates all required files itself. Here is new webrev: http://cr.openjdk.java.net/~ptisnovs/jtreg-jdk-test-LastErrorString-fix2/ I've also ran this test on Windows XP SP2 and it seems to works fine. Output of this test is stored in an attachment [stderr_windows.log]. Please note that I found only Win XP configured with Czech locale so some system messages might me incomprehensible, but these messages are correct ("Přístup byl odepřen" == "Access denied" etc.) Cheers, Pavel Work directory: ./ Deleting old file ./readable_file with result: true Deleting old file ./writable_file with result: true Deleting old file ./unwritable_file with result: true Creating new file ./readable_file with result: true Creating new file ./writable_file with result: true Creating new file ./unwritable_file with result: true File.createNewFile java.io.IOException: Systém nemůže nalézt uvedenou cestu WARNING: No exception for File.getCanonicalpath FileInputStream(file) java.io.FileNotFoundException: c:\pagefile.sys (Proces nemá přístup k souboru, neboť jej právě využívá jiný proces) FileInputStream(dir) java.io.FileNotFoundException: . (Přístup byl odepřen) FileInputStream.read() java.io.IOException: Read error FileInputStream.read(byte[]) java.io.IOException: Read error FileInputStream.skip java.io.IOException: Neplatný popisovač FileInputStream.available java.io.IOException: Neplatný popisovač FileOutputStream java.io.FileNotFoundException: .\unwritable_file (Přístup byl odepřen) FileOutputStream.write() java.io.IOException: Write error FileOutputStream.write(byte[]) java.io.IOException: Write error RandomAccessFile java.io.FileNotFoundException: c:\pagefile.sys (Proces nemá přístup k souboru, neboť jej právě využívá jiný proces) RandomAccessFile.getFilePointer java.io.IOException: Neplatný popisovač RandomAccessFile.length java.io.IOException: Neplatný popisovač RandomAccessFile.seek java.io.IOException: Neplatný popisovač RandomAccessFile.setLength java.io.IOException: Neplatný popisovač RandomAccessFile.readShort java.io.IOException: Read error RandomAccessFile.readInt java.io.IOException: Read error RandomAccessFile.writeShort java.io.IOException: Přístup byl odepřen RandomAccessFile.getFilePointer java.io.IOException: Přístup byl odepřen
Pavel Tisnovsky wrote:
:
I changed this regression test so it does not need to use shell script at all because now it creates all required files itself.
Here is new webrev: http://cr.openjdk.java.net/~ptisnovs/jtreg-jdk-test-LastErrorString-fix2/
I've also ran this test on Windows XP SP2 and it seems to works fine. Output of this test is stored in an attachment [stderr_windows.log].
Please note that I found only Win XP configured with Czech locale so some system messages might me incomprehensible, but these messages are correct ("Přístup byl odepřen" == "Access denied" etc.)
Thanks for changing the approach to avoid the shell script. Also thanks for testing it on a Windows machine, unpalatable as it might be to have to do that. If I read the test correctly then it will now leave unreadable and unwritable turds in the working directory. I see you clean up on re-run but it would be nicer if the test just cleaned up after itself. It would be nice to remove the attempt access of c:/pagefile.sys and z:/fooBAR/baz/GORP while you are there. That will simplify preparePaths as the runUnderWindows parameter won't be needed. As the work directory is the current directory then workDirParam isn't really needed either if you'd like to get rid of that too. Minor comment is that the comment on line 144 references the shell script that isn't in this version. Also, is the @SuppressWarnings("unused") at line 55 needed? -Alan.
participants (3)
-
Alan Bateman
-
Dr Andrew John Hughes
-
Pavel Tisnovsky