RFR(xs): (aix but affects shared code too) 8186665: buffer overflow in Java_java_nio_MappedByteBuffer_isLoaded0

[Peter Levart]

Hi Thomas,

Just one more concern...

On 10/18/2017 09:44 AM, Thomas Stüfe wrote:
> Hi all,
>
> I am wrapping up fixes which did not make it into the repo before the 
> consolidation. Alan already reviewed the last webrev, but I need a 
> second reviewer.
>
>
> Issue: https://bugs.openjdk.java.net/browse/JDK-8186665 
> <https://bugs.openjdk.java.net/browse/JDK-8186665>
> Last Webrev: 
> http://cr.openjdk.java.net/~stuefe/webrevs/8186665-buffer-overflow-in-mincore/webrev.01/webrev 
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186665-buffer-overflow-in-mincore/webrev.01/webrev/>
>
> Issue text for your convenience:
>
> --
> In Java_java_nio_MappedByteBuffer_isLoaded0, we call mincore(2) to 
> retrieve the paging status of an address range.
>
> The size of the output buffer for mincore(2) depends on the number of 
> pages in *system page size* in the given memory range (this is spelled 
> out more or less explicitly on AIX and Linux, nothing is said on 
> BSD/OSX, but I assume the same). The number of pages in the memory 
> range is calculated by MappedByteBuffer.isLoaded() and handed down to 
> Java_java_nio_MappedByteBuffer_isLoaded0() together with the memory 
> range to test.
>
> MappedByteBuffer.isLoaded() calculates this number of pages based on 
> jjdk.internal.misc.Unsafe.pagesize(), which ultimately comes down to 
> os::vm_page_size().
>
> For AIX, os::vm_page_size() may return a page size larger than the 
> system page size of 4K. The reason for this is that on AIX, memory can 
> be backed by different page sizes, usually either 4K or 64K - e.g. 
> posix thread stacks may have 4K pages, java heap (system V shared 
> memory) with 64K pages, but mmap memory is always 4K page backed...

If this is true and Unsafe.pagesize() returns a value > 4K, then perhaps 
also the MappedByteBuffer.load() method is wrong for AIX?

     public final MappedByteBuffer load() {
         checkMapped();
         if ((address == 0) || (capacity() == 0))
             return this;
         long offset = mappingOffset();
         long length = mappingLength(offset);
         load0(mappingAddress(offset), length);

         // Read a byte from each page to bring it into memory. A checksum
         // is computed as we go along to prevent the compiler from 
otherwise
         // considering the loop as dead code.
         Unsafe unsafe = Unsafe.getUnsafe();
         int ps = Bits.pageSize(); // << LOOK HERE
         int count = Bits.pageCount(length);
         long a = mappingAddress(offset);
         byte x = 0;
         for (int i=0; i<count; i++) {
             x ^= unsafe.getByte(a);
             a += ps; // << AND HERE
         }
         if (unused != 0)
             unused = x;

         return this;
     }

...this loop reads a byte from the start of each block in lumps of 
Bits.pageSize(). Should it always read in lumps of 4K for AIX? Do we 
rather need a special Unsafe.mmappedPageSize() method instead of just a 
hack in isLoaded0 ?

Regards, Peter

>
> But as the OpenJDK code base generally assumes one homogeneous page 
> size for everything - which is usually synonymous with 
> os::vm_page_size() - a decision had to be made which page size to 
> assume as a global system page size, and this may be a larger page 
> size than the 4K system page size mincore(2) assumes.
>
> This usually is no problem, but with mincore(2) it is: as the size of 
> the output buffer depends on the number of pages, calculating with a 
> too-large page size causes the output buffer to be too small and hence 
> the buffer overflows. The solution must be to base the size of the 
> mincore output buffer on the system page size.
>
> --
>
> Thanks and Kind Regards, Thomas
>
>
> On Thu, Aug 31, 2017 at 9:39 PM, Alan Bateman <Alan.Bateman at oracle.com 
> <mailto:Alan.Bateman at oracle.com>> wrote:
>
>     On 31/08/2017 19:01, Thomas Stüfe wrote:
>
>         Hi Alan,
>
>         thank you for your review!
>
>         Updated webrev:
>         http://cr.openjdk.java.net/~stuefe/webrevs/8186665-buffer-overflow-in-mincore/webrev.01/webrev/
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186665-buffer-overflow-in-mincore/webrev.01/webrev/>
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186665-buffer-overflow-in-mincore/webrev.01/webrev/
>         <http://cr.openjdk.java.net/%7Estuefe/webrevs/8186665-buffer-overflow-in-mincore/webrev.01/webrev/>>
>
>         I fixed the indention and embellished the comments around the
>         sentinel at the end of the buffer somewhat.
>
>     This looks okay to me.
>
>     -Alan
>
>