Meziu / rust-horizon

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
3 stars 1 forks source link

File not readable #11

Closed Meziu closed 2 years ago

Meziu commented 2 years ago

I was testing path and fs for #6.

Turns out, reading files is broken: doing so will result in a failed memory allocation. Looking into it it seems an issue with fstat, since it returns a wrong st_size value (always 1644524307), which the Rust File will then try to allocate into a Vec. That'd be about 1.67 GB of memory, so the system just dies.

@ian-h-chamberlain @AzureMarker

AzureMarker commented 2 years ago

I remember doing some debugging in this area when the "is_dir" check was failing in file-explorer. I think the underlying cause was the two std from static linking situation we had at that time, but I'd double check that the data structures line up between libc and the 3DS/libctru's definition.

Meziu commented 2 years ago

Hmm, it looks to be an issue only present with romfs files… I’ll search around the libctru docs.

Meziu commented 2 years ago

Yes, the various FileSystems don’t have full support of the normal posix functions. It looks like having the Romfs struct for using it as a normal FS is useless, since most of the functions used by std::fs aren’t available for that (including writing, now that I think about it…). We should work with ctru::services::fs instead.

ian-h-chamberlain commented 2 years ago

Hmm, some of this is a bit surprising to me, since I have been working on a change for colored test output which uses the romfs to create TERMINFO files and feed them to the std test runner (using std APIs of course). It seems to work as expected for the most part, although I do recall hitting a system crash on the first attempt I made.

I wonder what's different there vs the testing you've been doing. Perhaps the file size is relevant? In my case the file being opened is only 1462 bytes.

Looking at the test code that's running, it's in this codepath and the only check before opening it is fs::metadata(&p).is_ok():

    fn _from_path(path: &Path) -> Result<TermInfo, Error> {
        let file = File::open(path).map_err(Error::IoError)?;
        let mut reader = BufReader::new(file);
        parse(&mut reader, false).map_err(Error::MalformedTerminfo)
    }
Meziu commented 2 years ago

Welp, while it is great to have support for RomFS in the std::fs, it is surely prone to errors in any case: not being able to write, having to manually load the module, many features not working. It is interesting to have, but maybe we should focus on using the system specific functionality via ctru::fs, including RomFS.

AzureMarker commented 2 years ago

I'd like to take a look at this before we close out possibility of using std. The write issue shouldn't be a problem because it could just return an error when you try to open a file for writing (read only file systems already exist).

Meziu commented 2 years ago

I am looking into making a ctru::fs example. Having both would be cool, my example will include sdmc code too, it's just to test the module.

Meziu commented 2 years ago

Ok, looks like Romfs is just a big issue. Ironically, it doesn't work with FSUSER functions either. As someone from the Homebrew Discord said:

ARCHIVE_ROMFS gives you the raw romfs binary and the application is supposed to parse it.
Ctrulib does that, and makes it available through posix functions

This effectively means that (unless you are a god Homebrew programmer) Romfs ONLY works under std::fs, even though it has limited support. This is weird... I guess we should just test the whole std::fs module and see what we get...

AzureMarker commented 2 years ago

I wonder if we'll need to make libctru changes. At least it seems like they're still somewhat active, as there was a recent release.

Meziu commented 2 years ago

I wonder if we'll need to make libctru changes. At least it seems like they're still somewhat active, as there was a recent release.

I bumped ctru-sys, and that’ll be it most of the times. They usually only push breaking changes in major releases, though it depends.

Meziu commented 2 years ago

@ian-h-chamberlain could you make a working romfs example for ctru-rs, just so I can check my toolchain is alright.

ian-h-chamberlain commented 2 years ago

@ian-h-chamberlain could you make a working romfs example for ctru-rs, just so I can check my toolchain is alright.

Interesting... I started trying to extend the file-explorer example to read and display file contents and I found the same issue when using fs::read_to_string (fails to allocate)... However, it seems that BufReader avoids the problem since it doesn't allocate up front, and I am able to use it successfully. I have a couple more bugs to iron out and I'll push what I have. I think we can debug from there.

Edit: I think we might also need to reconsider the path scheme (or add OS-specific helpers), since I have noticed some weird behavior with .parent() and .pop().... and the romfs:/ seems a bit nonstandard for unix-like OSes.

Edit: seems like RedoxOS already uses a similar scheme (and has some support in sys::path), so maybe we can piggyback off of that: https://doc.redox-os.org/book/ch04-06-schemes.html

Meziu commented 2 years ago

Interesting... I started trying to extend the file-explorer example to read and display file contents and I found the same issue when using fs::read_to_string (fails to allocate)... However, it seems that BufReader avoids the problem since it doesn't allocate up front, and I am able to use it successfully.

Guessed so. It looks like a problem with “direct reading” functions that don’t have a buffer. We should find a way to get the filesize on romfs if fstat really doesn’t work.

ian-h-chamberlain commented 2 years ago

Ok, I did a little more digging, and it seems like stat called directly from 3ds-examples does what you would expect it to, so I suspect an FFI issue or similar here. Still trying to track it down, but it seems like we should be able to use those APIs since they are registered by libctru for the romfs "device", and supported in newlib.

Meziu commented 2 years ago

Ok, I did a little more digging, and it seems like stat called directly from 3ds-examples does what you would expect it to, so I suspect an FFI issue or similar here. Still trying to track it down, but it seems like we should be able to use those APIs since they are registered by libctru for the romfs "device", and supported in newlib.

std calls fstat though. Is that supported?

ian-h-chamberlain commented 2 years ago

Ok, I did a little more digging, and it seems like stat called directly from 3ds-examples does what you would expect it to, so I suspect an FFI issue or similar here. Still trying to track it down, but it seems like we should be able to use those APIs since they are registered by libctru for the romfs "device", and supported in newlib.

std calls fstat though. Is that supported?

Yeah, all the device-specific callbacks are registered here

I've made a little progress, I think there is an ABI mismatch somewhere on the stat type itself, where maybe gid_t is the wrong size?

// C:
{
    st_dev = 0,
    st_ino = 0,
    st_mode = 16676,
    st_nlink = 4,
    st_uid = 0,
    st_gid = 0,
    st_rdev = 0,
    st_size = 24,
    st_atim = {
        tv_sec = 1644690556,
        tv_nsec = 0
    },
    st_mtim = {
        tv_sec = 1644690556,
        tv_nsec = 0
    },
    st_ctim = {
        tv_sec = 1644690556,
        tv_nsec = 0
    },
    st_blksize = 512,
    st_blocks = 1,
    st_spare4 = {0, 0}
}

// Rust:
libc::unix::newlib::stat {
    st_dev: 0,
    st_ino: 0,
    st_mode: 16676,
    st_nlink: 4,
    st_uid: 0,
    st_gid: 0,
    st_rdev: 24,
    st_size: 1644690556,
    st_atime: 0,
    st_spare1: 1644690556,
    st_mtime: 0,
    st_spare2: 1644690556,
    st_ctime: 0,
    st_spare3: 512,
    st_blksize: 1,
    st_blocks: 0,
    st_spare4: [0, 0]
}

Edit: That was it! I'll post relevant PRs to libc + std shortly...