Crashing the osjs when uploading a file in osjs mountpoint

[maryam4s26]

hi, @andersevenrud According to the https://github.com/os-js/OS.js/issues/840 and https://github.com/os-js/osjs-server/pull/77, it seems that the error has been resolved. Unfortunately, the error has not been resolved.

[andersevenrud]

I can't seem to reproduce this after the update.

Can you share the logs with errors, etc. ?

[maryam4s26]

For me, crash can simply be happened by uploading a file in osjs mountpoint. node version: 16.0.0 osjs-server version: 3.4.0

error is :

Error: ENOENT: no such file or directory, open '/tmp/upload_727a4fa83a184e7c5fa9774d6349f0c2'
Emitted 'error' event on ReadStream instance at:

I think the error that happened should be solved in this way https://github.com/os-js/osjs-server/pull/78

@andersevenrud

[andersevenrud]

Huh.... suddenly I get this as well, and I'm a bit confused :)

Looking into it!

[andersevenrud]

I think I figured out what happens here and a solution.

1. A file is uploaded and a Stream is created
2. A VFS request is performed and a lookup happens
3. The found mountpoint performs some checks and throws an exception
4. The VFS cleans up all the temporary files from the upload
5. A response is given

Turns out that after 5 is finished the node runtime internally destroys all the created streams... and since 4 removed the file(s) an error event is emitted, which cannot be caught because it's totally out of scope.

This comes down to the implementation using formidable. The old version (that is used in this server) does not have any proper internal cleanup procedure (why I have done this in 4 myself) -- it only cleans up when an error happens; which again can cause a race condition; similar to what was solved in #77

I hope that made sense 😅

A easy way to get around this entirely is to defer the stuff done in 4, which I have done here: https://github.com/os-js/osjs-server/pull/80

Please try it out and see if it has solved this properly!

---

The correct fix for this is to refactor the upload stuff and use another (or newer) file upload library. But sadly, I don't have time for that.

[andersevenrud]

... hold on, I found another issue.

[andersevenrud]

The correct fix for this is to refactor the upload stuff

I went ahead and did a minor refactor here, and instead of deferring the cleanup the creation of the stream is deferred.

This way there is no chance a Stream is created if any exception occurs in the VFS request pre-checks.

Should be good enough for now. The PR has been updated.

[andersevenrud]

I actually think this might also have fixed an edge-case where temporary files never actually was cleaned up. I was messing around with one of my servers to try and reproduce this issue (which I actually did not manage... only on my Macbook at home) and saw there was quite a few old files that is probably from errors.

[andersevenrud]

I think the error that happened should be solved in this way https://github.com/os-js/osjs-server/pull/78

Just to comment on why I did not choose this. This solution prevents the crash from occurring, however it does not actually fix the reason for the crash, but rather "hides" the error from the system.

This means that any actual error with the stream (anything not related to the problem reported in this issue) would cause the requests to hang/fail. No error events ends with no exceptions, and no exceptions ends with no response :)

[maryam4s26]

I think the error that happened should be solved in this way #78

Just to comment on why I did not choose this. This solution prevents the crash from occurring, however it does not actually fix the reason for the crash, but rather "hides" the error from the system.

This means that any actual error with the stream (anything not related to the problem reported in this issue) would cause the requests to hang/fail. No error events ends with no exceptions, and no exceptions ends with no response :)

Thank you for your complete explanation.

[andersevenrud]

I have tested this patch for 30 minutes on a couple of different machines and it seems to work fine to me.

Now I really hope it also fixes it for you, and can't wait to hear your results from trying out the PR 🤞

Funny thing is that this bug is probably 4+ years old, so thank you very much for reporting 😅

[andersevenrud]

I went ahead and merged the PR and published a new 3.4.1 version. From my testing this seems to work.

[maryam4s26]

Hello @andersevenrud . Thanks for https://github.com/os-js/osjs-server/pull/80. I apologize for the delay, I have been testing https://github.com/os-js/osjs-server/pull/80. I tested this for osjs And it didn't crash anymore.

But according to this, I have my own adapter for vfs, and if an error occurs when using the writefile request, I will get the same error again and the system will crash. This also happened when I changed my node version to 16 and it didn't improve when I updated the server version to 3.4.1.

[andersevenrud]

But according to this, I have my own adapter for vfs, and if an error occurs when using the writefile request, I will get the same error again and the system will crash.

Yeah, sadly my PR does not solve this issue when an error occurs further down the line in this context. I started on a new patch to refactor parts and use a new version of formidable, but don't really have the time to finish it until this weekend.

I'll send you a notification :)

[maryam4s26]

Thank you very much.

[andersevenrud]

I just got an idea for a possible simpler solution. Could you test this PR?

https://github.com/os-js/osjs-server/pull/81

[maryam4s26]

Finallllllllllllly, it was fixed. Thank you very much.

[andersevenrud]

Sweet! I published a new version.

[maryam4s26]

Why do you only increment the version in osjs-server? Why don't you make any changes in osjs?

[andersevenrud]

I don't really feel that it's needed / I'm lazy, hehe. The installation instructions contains a step for updating packages.