Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle file streams differently #947

Closed
6 tasks
aviramha opened this issue Jan 16, 2023 · 3 comments
Closed
6 tasks

Handle file streams differently #947

aviramha opened this issue Jan 16, 2023 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@aviramha
Copy link
Member

aviramha commented Jan 16, 2023

Right now we do weird stuff with file streams, we should handle those similar to DirStreams - an object that is identified by usize (pointer) and points to a remote object (RemoteFile).
For example, on calling fclose we call fclose even if it's a stream managed by us, meaning this would most probably crash

This has not been tested yet, but we assume that running even simple flows with filestreams (e.g. by running mirrord on a node app) would break.

Functions to hook/fix

  • ferror - right now returns last os error but not os error of the specific stream meaning we're probably returning something invalid.
  • feof - need to hook
  • clearerr - need to hook
  • fileno - need to fix implementation
  • fdopen - check mode matches existing open file
  • fclose - need to fix

My proposed idea would be that most of the logic is layer-only, meaning we need some additional abstraction for fds similar to what libc provides:

#[derive(Default)]
pub(crate) struct FileStream {
    /// Raw Fd to use for operations (should be one managed by mirrord)
    pub(crate) fd: RawFd,
    /// Last error for `ferror` implementation
    pub(crate) last_error: c_int,
    /// is EOF for `feof` implementation
    pub(crate) is_eof: bool,
    /// for `flockfile`/`stuff_unlocked` implementation - skipped for now, leaving this
    /// in case we need to implement it later on.
    // pub(crate) lock: parking_lot::Mutex<()>,
}
@aviramha aviramha added the bug Something isn't working label Feb 1, 2023
@aviramha aviramha assigned aviramha and unassigned aviramha Feb 1, 2023
@aviramha
Copy link
Member Author

We needed to comment out flask http tests because those started failing with Fatal error: glibc detected an invalid stdio handle after most probably an update to werkzeug.

@meowjesty
Copy link
Member

meowjesty commented Mar 21, 2023

Looks like the culprit was __fsetlocking that would de-reference the * FILE, which we never filled with anything (as we were just passing it as a pointer address), you can see it in this implementation of it.

This all starts from how gethostname opens some network files using fopen and how __resolv_conf_load will end up trying to use __fsetlocking while reading /etc/resolv.conf.

This was referenced Mar 21, 2023
@aviramha
Copy link
Member Author

fixed in #1201

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants