OverlayFS Doesn't Throw Error When Not Initialized

[jbaicoianu]

I'm seeing this with em-dosbox, but I would guess it applies everywhere. When I delete a file from inside of Dosbox, it terminates immediately and throws the following exception:

TypeError: Cannot read property 'writeSync' of null : Error
    at jsStackTrace (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:1:20865)
    at stackTrace (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:1:21048)
    at Object.FS.handleFSError (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:1:1241956)
    at _unlink (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:1:1362507)
    at Array.iL (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:11:206153)
    at Aaa (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:7:85168)
    at Array.Rw (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:9:208120)
    at gi (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:5:40798)
    at gi (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:5:5908)
    at gi (https://ia802303.us.archive.org/cors_get.php?path=%2F34%2Fitems%2Fjsmess_engine_v2%2Fdosbox.js.gz:5:5908)

[jbaicoianu]

I should clarify that this is with ZipFS

[jvilk]

ZipFS is read only. If you want to write to it, you need to wrap it in an OverlayFS.

[jvilk]

Feel free to reopen if you're already using OverlayFS.

[perimosocordiae]

Seems like there could be a better error message, in any case.

[jvilk]

The thing is: BFS does have error messages for this error. writeSync throws if you invoke it with a file descriptor not opened with a writeable mode, and openSync throws if you try to open something w/ a writeable mode on a read-only file system.

I have a feeling Emscripten is swallowing whatever BFS is throwing, and is just barreling through with something crazy. e.g. opening the file with a write mode failed, but it tried to write to it anyway despite the failure.

If @jbaicoianu can find where BFS throws an error and sees that it should throw something better, I'd be happy to investigate further.

[jbaicoianu]

@jvilk thing is, this isn't a file open/write scenario - it's just plain unlink(). To my knowledge nothing is trying to write to a file descriptor, this happens when I boot up dosbox and just type "del blah.txt". In native dosbox, if this were a read-only FS, the FS layer would return an EACCESS error, and dosbox would say "Unable to delete: foo.txt"

Also - we are using OverlayFS with em-dosbox. I think in the file write scenario you're right it would work, but does OverlayFS handle unlinked files properly (eg, keep a blacklist of deleted files and prevent the app from seeing their entries in the underlying ZipFS)?

[jvilk]

@jbaicoianu yes, it does. it has a deletion log.

https://github.com/jvilk/BrowserFS/blob/master/src/backend/OverlayFS.ts#L9

But I don't know what I'm supposed to do with the information you've provided me. I don't know where that stack trace is coming from. writeSync is a function on a number of BFS objects. Is there code online somewhere that demonstrates the issue?

[jbaicoianu]

Sure. https://archive.org/details/PSP311 - this will load to the DOS prompt. Then, just run "del wpg.flt" and it'll freeze, with the stack trace above in the console.

[jbaicoianu]

As far as a simplified code sample, we don't have one yet - I was hoping that "browserfs throws exception while unlinking files" would maybe trigger some ideas in your mind, since you're more familiar with how the NodeFS stuff works under the hood. If this isn't enough to go on then I can try to create a super simplified test case, but might take a couple days for me to get around to it.

[jbaicoianu]

I believe this is the C++ code which is triggering the error:

https://github.com/dreamlayers/em-dosbox/blob/bb94d5c4fbcc575a4148cd709366a9c04673de78/src/dos/drive_local.cpp#L154-L193

[jvilk]

I'll take a look at this tomorrow. Sorry if my answer sounded snappy -- I honestly didn't know where to start, as I explicitly remembered adding a ton of descriptive error messages for this exact scenario. Debugging within the Emscripten crazy-fest gets... interesting... as it typically crashes devtools. But I'll give it a shot.

[jbaicoianu]

I understand the frustration, debugging emscripten errors is definitely not fun :) If you need any help let me know. I can compile a version without optimizations if you don't see anything obvious right off the bat.

[jvilk]

@jbaicoianu figured it out. The problem is that initialize is never called on OverlayFS, so it never initializes its delete log. As a result, when you try to delete something and write to the delete log, it fails because it never opened the delete log!

Here's where the delete log is set.

If you are working on EM-DOSBox, you should make sure it initializes the OverlayFS before it uses it.

[jvilk]

I verified that this.isInitialized is false when the exception is thrown, confirming that this is the problem.

[jbaicoianu]

Great, thanks! @db48x I assume this would happen in the Emularity code, does this look easy enough to fix?

[jvilk]

It should happen right after this line, halting progress in the script until it completes w/o errors.

[jvilk]

Going back in history, it looks like I'm the one that forgot to do this in the first place. D'oh! :)

[db48x]

Should it be .OverlayFS(foo, bar).initialize()?

[jvilk]

They don't chain, so it should be:

game_data.fs = new OverlayFS(foo, bar);
game_data.fs.initialize(function(e) {
  if (e) {
    // There was an error! :(
  } else{
    // Everything is cool, OverlayFS is ready to use!
  }
});

[db48x]

Ah. The error case can just reject(), and the success case can wrap the rest of the function. Unless I'm forgetting something.

[jvilk]

That sounds about right!

[jvilk]

Let me know if you need any further information. Next time I revisit BFS for a release, I'll add in error messages regarding initialization.

[jvilk]

The latest commit now throws an exception if you try to use OverlayFS or AsyncMirror without initializing them first.