Round 2 Code Review request for new MallocMaxBytes option and fixed test (7030610, 7037122, 7123945)

[David Holmes]

Hi Ron,

There is a type mismatch between MallocMaxTestWords (uintx) and 
cur_malloc_words (juint). The former will be 64-bit on 64-bit systems, 
while the latter is always 32-bit.

os.cpp:

  580   if (MallocMaxTestWords > 0 &&
  581       (cur_malloc_words + (alloc_size / BytesPerWord)) > 
MallocMaxTestWords) {
  582     return false;
  583   }

this calculation can overflow if MallocMaxWords is close to the limit. 
Now I doubt we can actually allocate that much but the expression can be 
rearranged to avoid overflow.

Not sure I see the point of passing ptr to the add function instead of 
just doing:

if (ptr != null)
    add_current_malloc_test_words(alloc_size);

Otherwise seems functionally ok but I really hate to see test-only code 
being injected into a primary execution path like this. Why not move it 
off to the side:

if (MallocMaxTestWords > 0)
    return testMalloc(alloc_size);

and have

void* testMalloc(size_t alloc_size) {
   if (!check_malloc_max_test_words(alloc_size)) return NULL;
   u_char* ptr = (u_char*)::malloc(alloc_size);
   add_current_malloc_test_words(alloc_size, ptr);
   return ptr;
}

For the testcase I only skimmed it. Not sure of the logic there, but the 
proof of the test is in the testing so I assume you have managed to 
tickle all the different exit modes for the test?

Cheers,
David

On 16/03/2013 4:29 AM, Ron Durbin wrote:
> Dan
>
> I have a round 2 code review ready for:
>
> Round 2 addresses:
>
> - addressed code review comments from Coleen, Dan and David
> - note: src/share/vm/runtime/os.hpp is no longer changed.
>
> Web URL http://cr.openjdk.java.net/~rdurbin/7030610-webrev-cr1/1-hsx25/
>
> Internal URL http://javaweb.us.oracle.com/~rdurbin/7030610-webrev/1-hsx25/
>
> 7030610 runtime/6878713/Test6878713.sh fails, Error. failed to clean up
> files after test.
>
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7030610
>
> https://jbs.oracle.com/bugs/browse/JDK-7030610
>
> 7037122 runtime/6878713/Test6878713.sh is written incorrectly
>
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7037122
>
> https://jbs.oracle.com/bugs/browse/JDK-7037122
>
> (This fix only addresses the issues with test
> runtime/6878713/Test6878713.sh;
>
> 7037122 names other tests that are not addressed by this fix)
>
> 7123945 runtime/6878713/Test6878713.sh require about 2G of
>
> _http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7123945_
>
> https://jbs.oracle.com/bugs/browse/JDK-7123945
>
> Summary: Add new diagnostic option -XX:MallocMaxTestWords=NNN and fix
>
> Test6878713.sh. Test6878713.sh is a security regression test with four
> different
> failure modes.Three are documented in the defects above and the forth
>
> was found during bug triage. The root cause of these failures can be
>
> traced back to two issues: Inconsistent test condition setup and error
> handling.
>
> All four defect manifestations are fixed by this set of changes.
>
> The testing is not completed because the JPRT run is running slowly.
>