Remove Test8050807.sh?
Gerald Thornbrugh
gerald.thornbrugh at oracle.com
Tue Feb 27 21:15:15 UTC 2018
OK, sounds good.
On 02/27/2018 02:13 PM, Daniel D. Daugherty wrote:
> On 2/27/18 3:54 PM, harold seigel wrote:
>>
>> Yes, I meant a normal RFR to hotspot rt confidential.
>>
>
> Cool.
>
>
>> I haven't asked any of the security folks about it. I'm not sure who
>> else would review it other than possibly David Holmes. Or Karen, if
>> I asked her to.
>>
>
> Here's the original changeset with reviewers listed:
>
> changeset: 16218:ae97beda7b49
> user: gthornbr
> date: Mon Nov 17 15:51:15 2014 -0500
> files: closed/hotspot/test/closed/runtime/8050807/Test8050807.sh
> closed/hotspot/test/closed/runtime/8050807/test.java
> description:
> 8050807: Better performing performance data handling
> Reviewed-by: dcubed, pnauman, ctornqvi, dholmes, mschoene
> Contributed-by: gerald.thornbrugh at oracle.com
>
>
> Of course, Jerry and I will both officially review... :-)
>
> Dan
>
>
>> Harold
>>
>>
>> On 2/27/2018 3:50 PM, Daniel D. Daugherty wrote:
>>> On 2/27/18 3:22 PM, harold seigel wrote:
>>>>
>>>> Thanks Dan and Jerry! This is most helpful.
>>>>
>>>> I tried "/manual" but that did not prevent the test from running
>>>> when doing "jtreg ... hotspot/runtime". I also didn't see how
>>>> "/manual" would protect against other Java processes for the same
>>>> user that were run outside of the test's JTReg run.
>>>>
>>>> I think the test should PASS for the (hopefully unusual) process
>>>> interference case. Then, no-one will need to look at the result.
>>>> That isn't great but better than the current case where the test is
>>>> excluded and not run at all.
>>>>
>>>> Should I go out for public review with test2 deleted and using
>>>> Dan's code for test1 ?
>>>>
>>>
>>> By "public" you don't mean OpenJDK right? This is a closed security
>>> test so this one has to be internal...
>>>
>>> I would be okay with a wider internal review. Have you asked any
>>> of the security devs what should be done with a test that is so
>>> very picky?
>>>
>>> Dan
>>>
>>>
>>>> Thanks, Harold
>>>>
>>>>
>>>> On 2/27/2018 3:15 PM, Gerald Thornbrugh wrote:
>>>>> Hi Dan,
>>>>>
>>>>>> On Feb 27, 2018, at 12:32 PM, Daniel D. Daugherty
>>>>>> <daniel.daugherty at oracle.com
>>>>>> <mailto:daniel.daugherty at oracle.com>> wrote:
>>>>>>
>>>>>> I caught up with the review thread for 8067033 so now I see why that
>>>>>> second block is removed. I'm not sure about what 8067033 is claiming.
>>>>>> The second test case doesn't create a non-hardlinked regular file in
>>>>>> the target directory because the VM is allowed to remove that kind
>>>>>> of file. I didn't think it was allowed to remove a hardlinked file
>>>>>> nor a symlinked file. However, it has been a long time since I've
>>>>>> thought about the original fix.
>>>>>>
>>>>>> Jerry should probably chime in here about the removal of the second
>>>>>> test case.
>>>>> The second test case is ok to remove because just before putback I
>>>>> change the code to remove
>>>>> symlinks and hardlinks because it was ok to do so.
>>>>>>
>>>>>> As for replacing:
>>>>>>
>>>>>> rmdir ${HSPERFUSERPATH}
>>>>>>
>>>>>> with:
>>>>>>
>>>>>> rm -rf ${HSPERFUSERPATH}
>>>>>>
>>>>>> If you do that, then this test can interfere with another running
>>>>>> VM that happens to have a file in ${HSPERFUSERPATH}. The other VM
>>>>>> can potentially crash when its JVM stat files are removed out from
>>>>>> under it.
>>>>> This is correct, I specifically used “rmdir” because of
>>>>> interaction with another test.
>>>>>>
>>>>>> Basically, you want something like this:
>>>>>>
>>>>>> rmdir ${HSPERFUSERPATH}
>>>>>> ln -s ${DIRTARGETPATH} ${HSPERFUSERPATH}
>>>>>> if [ ! -h ${HSPERFUSERPATH} ]; then
>>>>>> # Was not able to create HSPERFUSERPATH as a symlink
>>>>>> # so we cannot execute this part of the test.
>>>>>> echo "Warning: Existing ${HSPERFUSERPATH} is interfering
>>>>>> with this test."
>>>>>> echo "This test is PASSING spuriously."
>>>>>> # Cleanup what 'ln -s' created:
>>>>>> rm ${HSPERFUSERPATH}/${DIRTARGETPATH}
>>>>>> # Fall thru to rest of the cleanup code.
>>>>>> else
>>>>>> # The "test 1" stuff goes here.
>>>>>> fi
>>>>>>
>>>>>> Here's what you get out of this version:
>>>>>>
>>>>>> - The "rmdir" line will only remove an empty directory so this test
>>>>>> won't mess with another running VM.
>>>>>> - The symbolic link creation will either win the race with a VM
>>>>>> trying to create the ${HSPERFUSERPATH} directory or it will
>>>>>> lose the race.
>>>>>> - If ${HSPERFUSERPATH} is a symlink, then the "test 1" logic can
>>>>>> execute.
>>>>>>
>>>>>>
>>>>>> Maybe a spurious PASS is not what we want. Maybe we want an
>>>>>> ERROR result instead of a spurious PASS or a FAIL. Dunno.
>>>>> I am not sure either. This test is difficult because of the need
>>>>> to run it in parallel with other tests.
>>>>>
>>>>> Did you ever determine if the “manual” option would do what we want?
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Jerry
>>>>>>
>>>>>> Dan
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2/27/18 1:56 PM, Daniel D. Daugherty wrote:
>>>>>>> Why is the second test scenario removed from the test?
>>>>>>>
>>>>>>> Dan
>>>>>>>
>>>>>>>
>>>>>>> On 2/27/18 1:32 PM, harold seigel wrote:
>>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> I looked into the possibility of detecting when the test was
>>>>>>>> going to fail because of interference from another Java
>>>>>>>> process, and, in that case, then ignoring the result. I came
>>>>>>>> up with the following:
>>>>>>>>
>>>>>>>> http://javaweb.us.oracle.com/~hseigel/webrev/bug_8067033.Feb27/webrev/index.html
>>>>>>>>
>>>>>>>> I think the test's "other Java process failure" only occurs if
>>>>>>>> another process creates the "/tmp/hsperfdata_<user>" after the
>>>>>>>> test deletes it but before the test re-creates it as a symbolic
>>>>>>>> link at line 89.
>>>>>>>>
>>>>>>>> So, I added the "if [ -h ${HSPERFUSERPATH} ]" check at line 112
>>>>>>>> in order to ignore the cases where another process created
>>>>>>>> "/tmp/hsperfdata_<user>" as a regular directory before the test
>>>>>>>> could create the symbolic link.
>>>>>>>>
>>>>>>>> I tested this by temporarily adding "mkdir ${HSPERFUSERPATH}"
>>>>>>>> just before line 89.
>>>>>>>>
>>>>>>>> Does this seem okay?
>>>>>>>>
>>>>>>>> Thanks, Harold
>>>>>>>>
>>>>>>>> On 2/23/2018 3:52 PM, Gerald Thornbrugh wrote:
>>>>>>>>> Hi Coleen,
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If we run it and it fails when we run with jtreg on the
>>>>>>>>>> command line or through make, and it's not on the
>>>>>>>>>> ProblemList.txt, it is going to cost everybody time to figure
>>>>>>>>>> out the failure isn't theirs.
>>>>>>>>>>
>>>>>>>>>> One of the uses of these tests is for POC verification. If
>>>>>>>>>> we can put the test in the original BugDB bug, or it's
>>>>>>>>>> already there, that might meet the requirements of having
>>>>>>>>>> this test.
>>>>>>>>>>
>>>>>>>>>> I wonder why this is a security bug at all.
>>>>>>>>> It allowed a non privileged user to overwrite and remove the
>>>>>>>>> files associated with the
>>>>>>>>> owner of the /tmp/hsperfdata_<user> directory. If that user
>>>>>>>>> was "root", system files
>>>>>>>>> could be overwritten or removed.
>>>>>>>>>
>>>>>>>>> Jerry
>>>>>>>>>>
>>>>>>>>>> Coleen
>>>>>>>>>>
>>>>>>>>>> On 2/23/18 3:33 PM, harold seigel wrote:
>>>>>>>>>>> Thanks Dan.
>>>>>>>>>>>
>>>>>>>>>>> I'll try it and see what happens.
>>>>>>>>>>>
>>>>>>>>>>> Harold
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 2/23/2018 3:32 PM, Daniel D. Daugherty wrote:
>>>>>>>>>>>> I don't think that '/manual' tests are executed when we
>>>>>>>>>>>> specify these
>>>>>>>>>>>> "test groups". I could very well be wrong...
>>>>>>>>>>>>
>>>>>>>>>>>> When I said this:
>>>>>>>>>>>>
>>>>>>>>>>>>> I believe if you just run jtreg on a target directory, it
>>>>>>>>>>>>> runs
>>>>>>>>>>>>> everything in that hierarchy including the manual tests.
>>>>>>>>>>>>
>>>>>>>>>>>> what I meant was you manually ran "jtreg
>>>>>>>>>>>> closed/test/hotspot/jtreg/compiler/c2"
>>>>>>>>>>>> and it proceeds to run _everything_ in the "c2" hierarchy
>>>>>>>>>>>> including the manual
>>>>>>>>>>>> tests because you ran it from the cmd line, i.e., have a
>>>>>>>>>>>> controlling tty.
>>>>>>>>>>>>
>>>>>>>>>>>> It dawns on me that jtreg may not be that smart (detecting
>>>>>>>>>>>> controlling tty).
>>>>>>>>>>>>
>>>>>>>>>>>> I don't know for sure. Maybe /manual tests are only run
>>>>>>>>>>>> when asked for...
>>>>>>>>>>>>
>>>>>>>>>>>> Dan
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2/23/18 3:26 PM, harold seigel wrote:
>>>>>>>>>>>>> So, /manual won't stop this test from intermittently
>>>>>>>>>>>>> failing during mach5 and other test runs?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Harold
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 2/23/2018 3:24 PM, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>> I believe if you just run jtreg on a target directory, it
>>>>>>>>>>>>>> runs
>>>>>>>>>>>>>> everything in that hierarchy including the manual tests.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> However, I suspect that what you really want to know is
>>>>>>>>>>>>>> whether
>>>>>>>>>>>>>> SQE/SQA has a specific phase of their testing where
>>>>>>>>>>>>>> someone sits
>>>>>>>>>>>>>> down and runs jtreg with "-k manual" and checks the
>>>>>>>>>>>>>> results...
>>>>>>>>>>>>>> That would be a question for Misha...
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 2/23/18 2:31 PM, harold seigel wrote:
>>>>>>>>>>>>>>> That sounds good, but do the /manual tests ever get run?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Harold
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> On 2/23/2018 2:30 PM, Gerald Thornbrugh wrote:
>>>>>>>>>>>>>>>> This sounds like a good idea, I vote for Dan's approach.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> On 02/23/2018 12:20 PM, Daniel D. Daugherty wrote:
>>>>>>>>>>>>>>>>> Removing a test for a security vulnerability is
>>>>>>>>>>>>>>>>> problematic.
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Perhaps modify the test to include "/manual" so that
>>>>>>>>>>>>>>>>> it is only
>>>>>>>>>>>>>>>>> run when manual tests are run. Something like this:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> @run shell/manual Test8050807.sh
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> I see other tests do this:
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> closed/test/hotspot/jtreg/compiler/c2/4523683/TestCase.java:
>>>>>>>>>>>>>>>>> * @run shell/manual Test4523683.sh
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> Dan
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>> On 2/23/18 10:42 AM, Gerald Thornbrugh wrote:
>>>>>>>>>>>>>>>>>> The fix was for a security vulnerability. My fear is
>>>>>>>>>>>>>>>>>> that someone may ask "Are we still vulnerable
>>>>>>>>>>>>>>>>>> to this security issue?". How do we go about proving
>>>>>>>>>>>>>>>>>> that unless we still have a test available?
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I say definitely remove it. The trouble with
>>>>>>>>>>>>>>>>>>> putting it in a place for intermittent failures is
>>>>>>>>>>>>>>>>>>> that we'll never look at it, so it's as good as
>>>>>>>>>>>>>>>>>>> removed.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> I think this was a transient problem and not likely
>>>>>>>>>>>>>>>>>>> to reoccur.
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> Coleen
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>> On 2/23/18 10:03 AM, Gerald Thornbrugh wrote:
>>>>>>>>>>>>>>>>>>>> Hi Harold,
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Yes, the test is very problematic. The main reason
>>>>>>>>>>>>>>>>>>>> the test was putback was
>>>>>>>>>>>>>>>>>>>> to meet the requirement of adding a test with each
>>>>>>>>>>>>>>>>>>>> change.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> I have no problem removing the test.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Is there a place for standalone tests that are
>>>>>>>>>>>>>>>>>>>> problematic when they are run in
>>>>>>>>>>>>>>>>>>>> parallel? If there is a home for wayward test it
>>>>>>>>>>>>>>>>>>>> would be nice to place it there.
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Thanks!
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>> Jerry
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Any objection if I permanently remove test
>>>>>>>>>>>>>>>>>>>>> closed/test/hotspot/jtreg/runtime/8050807/Test8050807.sh
>>>>>>>>>>>>>>>>>>>>> ?
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> The test writes files to the
>>>>>>>>>>>>>>>>>>>>> "/tmp/hsperfdata_<user>/" and checks if the perf
>>>>>>>>>>>>>>>>>>>>> memory cleanup code removes files correctly.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> The problem with the test is that it requires
>>>>>>>>>>>>>>>>>>>>> exclusive access to directory
>>>>>>>>>>>>>>>>>>>>> "/tmp/hsperfdata_<user>/". So, even if JTReg ran
>>>>>>>>>>>>>>>>>>>>> the test stand-alone, the test could still fail if
>>>>>>>>>>>>>>>>>>>>> the same user was running a separate Java
>>>>>>>>>>>>>>>>>>>>> invocation on the same system, such as a Mach5
>>>>>>>>>>>>>>>>>>>>> task, another JTReg stream, etc..
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> The name "/tmp/hsperfdata_<user>/" is hard-coded
>>>>>>>>>>>>>>>>>>>>> in the JVM. So, changing the test to use a
>>>>>>>>>>>>>>>>>>>>> different directory is not an option.
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Currently, the test is on the Problem List. (See
>>>>>>>>>>>>>>>>>>>>> closed/test/hotspot/jtreg/ProblemList.txt.)
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>> Thanks, Harold
>>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-runtime-dev
mailing list