Remove Test8050807.sh?
Daniel D. Daugherty
daniel.daugherty at oracle.com
Tue Feb 27 21:13:07 UTC 2018
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