fix(mmap): handle over-allocated files #10526

petersalomonsen · 2020-02-21T17:36:25Z

fix for not writing buffer contents from expanded files beyond allocated memory

sbc100

Thanks for the fix!

A few style nits in the test but otherwise looks good.

sbc100 · 2020-02-21T18:23:19Z

tests/fs/test_mmap.c

+
+    char *map;
+
+    map = (char*)mmap(NULL, textsize, PROT_READ, 0, fd, 0);


these two lines can be a single line.

should be ok now

sbc100 · 2020-02-21T18:23:50Z

tests/fs/test_mmap.c

+  {
+    const char* path = "/yolo/expandedfile.txt";
+
+    int fd = open(path, O_RDWR | O_CREAT, (mode_t)0600);


Just inline path since its only used once.

should be ok now

sbc100 · 2020-02-21T18:24:11Z

tests/fs/test_mmap.c

+    size_t textsize = 33;
+
+    // multiple calls to write so that the file is expanded
+    for(int n=0;n<textsize;n++) {


space after for and between all the operators.

looking better now?

sbc100 · 2020-02-21T18:25:29Z

tests/fs/test_mmap.c

  }
+
+  /**
+   * MMAP to and expanded file


Is "expanded file" terminology that is used elsewhere? How about "over-allocated memfs file"?

I chose the term expanded file because the "expansion" is done by a method called expandFileStorage which you can see here:

emscripten/src/library_memfs.js

Line 115 in 0e4bb32

expandFileStorage: function(node, newCapacity) {

To me expanded file doesn't hold the right meaning. The expandFileStorage may or may not over-allocate, right? But its the fact the you over-allocate the storage that is key here IIUC, no? I could be wrong.. I didn't write the memfs code so maybe expanded file means more to the authors of this code.

when appending to a file expandFileStorage will increase the buffer size for that file more than the bytes requested to be written. This is to avoid having to increase the buffer for every call to write (which would be very slow). So instead the actual file size is tracked in a separate property called usedBytes.

mmap allocates memory based on the usedBytes, and calls javascripts ArrayBuffer set method on the heap with the files buffer as parameter. The set method has no parameter for the length, and so it copies the full contents of the file buffer. To avoid copying more than the actual file size the file buffer is sliced before calling set, but before this fix, slicing would only occur if the requested length was less than used bytes (which is mostly not the case if mapping the entire file). So instead the criteria for slicing must be if the requested length is less than the length of the file buffer - which is normally more than usedBytes for expanded files.

but yes - expandFileStorage does over-allocate file storage in that sense. Just wouldn't confuse it with over-allocating of heap, since the case here is that mmap writes beyond what it allocated on the heap.

not sure what the best term here is though. Maybe we need more opinions?

I guess you could call it "over-sized" if you prefer? But I think over-allocated is pretty clear and applies to any object that is larger than needs to be to make room for future expansion. We can let @kripken have the final say :)

I don't have a strong opinion on the term, but "over-allocated" does seem pretty reasonable, let's change to that?

Also and should be an in the comment.

petersalomonsen · 2020-02-21T19:29:26Z

thanks for the review @sbc100 :-) Would be great to have this merged.

libgit2/libgit2#5345 depends on this

sbc100 · 2020-02-21T19:31:35Z

@kripken can you give final lgtm here?

kripken

Nice, thanks @petersalomonsen !

kripken · 2020-02-25T17:08:49Z

tests/fs/test_mmap.c

  }
+
+  /**
+   * MMAP to and expanded file


I don't have a strong opinion on the term, but "over-allocated" does seem pretty reasonable, let's change to that?

Also and should be an in the comment.

fix for files with buffers larger than the file size where contents were written beyond memory allocated for the map fixes emscripten-core#10527

petersalomonsen · 2020-02-25T19:02:34Z

over-allocated it is then :-)

Thanks for reviewing!

petersalomonsen force-pushed the fix-mmap-expanded-files branch from 5b0585e to 0e4bb32 Compare February 21, 2020 17:39

petersalomonsen requested review from juj and kripken February 21, 2020 17:43

sbc100 approved these changes Feb 21, 2020

View reviewed changes

petersalomonsen force-pushed the fix-mmap-expanded-files branch from 0e4bb32 to 9034ec3 Compare February 21, 2020 19:20

petersalomonsen mentioned this pull request Feb 21, 2020

emscripten support libgit2/libgit2#5345

Closed

kripken approved these changes Feb 25, 2020

View reviewed changes

fix(mmap): handle over-allocated files

cf63447

fix for files with buffers larger than the file size where contents were written beyond memory allocated for the map fixes emscripten-core#10527

petersalomonsen force-pushed the fix-mmap-expanded-files branch from 9034ec3 to cf63447 Compare February 25, 2020 18:38

petersalomonsen changed the title ~~fix(mmap): handle expanded files~~ fix(mmap): handle over-allocated files Feb 25, 2020

kripken merged commit 73b81e2 into emscripten-core:master Feb 25, 2020

petersalomonsen deleted the fix-mmap-expanded-files branch February 25, 2020 19:43

petersalomonsen mentioned this pull request Feb 4, 2023

suggestion: use libgit2 under the hoods isomorphic-git/isomorphic-git#268

Closed


		char *map;

		map = (char*)mmap(NULL, textsize, PROT_READ, 0, fd, 0);

fix(mmap): handle over-allocated files #10526

fix(mmap): handle over-allocated files #10526

Uh oh!

Conversation

petersalomonsen commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petersalomonsen commented Feb 21, 2020

Uh oh!

sbc100 commented Feb 21, 2020

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

petersalomonsen commented Feb 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

petersalomonsen commented Feb 21, 2020 •

edited

Loading