Bug 13901 - Bug in System.IO.MemoryMappedFiles results in undersized views.
Summary: Bug in System.IO.MemoryMappedFiles results in undersized views.
Status: RESOLVED FIXED
Alias: None
Product: Class Libraries
Classification: Mono
Component: System.Core ()
Version: master
Hardware: PC Mac OS
: --- normal
Target Milestone: Untriaged
Assignee: Bugzilla
URL:
Depends on:
Blocks:
 
Reported: 2013-08-10 15:24 UTC by Brian Berry
Modified: 2014-06-19 16:57 UTC (History)
4 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:

Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on GitHub or Developer Community with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED FIXED

Description Brian Berry 2013-08-10 15:24:30 UTC
Found:  MemoryMappedFile.cs undersizes map views when it adjusts real mapping offsets to align with the system page size.

Steps to reproduce:  (this was using Xamarin.Android, but I think the problem is generic)
   * Create a memory mapped file for a file with a size significantly different than the system page size.
   * Attempt to map the tail end of the file and read its contents.

Result:
   * Garbage data acquired.

Notes:

We have been using a custom copy of the tip MemoryMappedFile.cs from github in order to dodge another unrelated MemoryMappedFile issue previously reported (https://bugzilla.xamarin.com/show_bug.cgi?id=12739).  Until such time as that prior issue is fixed/released, our only changes to the module are to call our own native getpagesize() implementation which is absent on Android.

The root cause of this new issue appears to be the logic where system mmap() calls are made.   E.g.:

        internal static unsafe void Map (int file_handle, long offset, ref long size, MemoryMappedFileAccess access, out IntPtr map_addr, out int offset_diff)
        {
            if (pagesize == 0)
                pagesize = mono_get_pagesize ();

            long fsize = mono_get_filesize_from_fd (file_handle);
            if (fsize < 0)
                throw new FileNotFoundException ();

            if (size == 0 || size > fsize)
                size = fsize;

            // Align offset
            long real_offset = offset & ~(pagesize - 1);

            offset_diff = (int)(offset - real_offset);

            map_addr = mono_mmap (IntPtr.Zero, (IntPtr) size, 
                             ToUnixProts (access),
                             access == MemoryMappedFileAccess.CopyOnWrite ? MAP_PRIVATE : MAP_SHARED,
                             file_handle, real_offset);
  
Note that to contend with system page-aligned access, a "real" offset is computed and the system mmap is invoked with this adjusted offset.  However, the same mmap is provided the unadjusted request size.   For a nonzero "offset_diff", this results in a mapping that is undersized by offset_diff.

Suggested fix:  In the two cases (check for others) in the module where mmap() calls are invoked, the offset_diff should be included in the mapping size to ensure the requested data are fully mapped, e.g.:

            map_addr = mono_mmap (IntPtr.Zero, (IntPtr) size+offset_diff, <--- ***ADDED***
                             ToUnixProts (access),
                             access == MemoryMappedFileAccess.CopyOnWrite ? MAP_PRIVATE : MAP_SHARED,
                             file_handle, real_offset);

Unit tests for both off-alignment and tail content reads for memory mapped files is also suggested.

Please let me know if you need any more info.
Many thanks!
Comment 1 Brian Berry 2013-08-12 13:11:58 UTC
Apologies, I was imprecise in my repro steps above, so let me take another swing.

The issue is when real_offset is different offset---that is to say the start of the user-requested mapping is not page-aligned.   Any internal downward adjustment of the mapping offset must be accompanied by an equal increase in internal mapping window size, or the tail of the user-requested region gets cut off.

My original comments regarding file size were not correct.  File size does not matter---offset does.  In my use case,  I was attempting to access a tail-located table of contents in a large database via memory mapped access.  Given that this TOC was located at an unaligned offset within the overall file, the tail of the TOC was not addressable as a result.
Comment 2 Brian Berry 2013-08-21 01:51:40 UTC
An additional problem exists downstream of original mapping in the non-page-aligned case as well.  I'll post here as it is related to the original issue and weakness in the module generally.

Previously discussed:
   * For a given user-requested offset UO and size US, the module must adjust for page alignment.
   * The module will have to use an aligned offset AO <= UO in order satisfy page-aligned mmap() calls.
   * If the user has requested page-aligned offset already, AO will equal UO and the module works fine.
   * If the user does NOT request a page-aligned offset, (UO-AO) will be positive.
   * In that case, the module currently shifts the mmap() downward by (UO-AO) bytes.
   * In doing so, it fails to also increase the mapped size by (UO-AO) to keep the full user-requested
     range mapped---the tail is dropped.

Additionally discovered to be broken in this case:

   * An asymmetry exists where MemoryMappedViewAccessor performs the MemoryMapImpl.Map()
     call but the SafeMemoryMappedViewHandle class is made responsible for the corresponding Unmap().
   * The former (MMVA) receives the *true* mmap()'ed address (and the "offset_diff", which is really
     just (UO-AO) in our description above.  This information is not provided to the latter (SMMVH).
   * SMMV then ultimately performs an Unmap() based on pointer/size related to the original *user*
     requested region, not the *aligned* region originally mapped.
   * This asymmetry results in not all pages being unmapped in many non-page-aligned cases,
     resulting in address space leaks / fragmentation / failures to further map big chunks due to
     errno 11 (EAGAIN) --- the observed symptom here.

I have locally changed the constructor to SafeMemoryMappedViewHandle to take the true mmap_addr, offset_diff information, store this for the purposes of a proper symmetric Unmap() call later.  (The proper unmap is at mmap_addr, and for (size + offset_diff) bytes provided that fixes were also made per "previously discussed" and my earlier reports above.

The asymmetry between responsibility here (one class doing the Map(), the other doing the Unmap()) is still undesirable and a little hazardous, in my opinion.

Please feel free to contact me if you would like to discuss any of this further.

Many thanks, everyone.
Comment 3 Alex Rønne Petersen 2014-01-06 15:58:40 UTC
Can you attach patch files with the changes you made? Or even better, create a pull request on GitHub?
Comment 4 Rodrigo Kumpera 2014-06-19 16:57:46 UTC
Fixed in master.