fix seeking - need to handle whence properly

[bavardage]

per docs, the whence parameter is set as in fseek

for example, it's not uncommon for this callback to be called with whence = 2, offset = -1 indicating seek 1 byte from the end of the stream

before this change, we'd cast this -1 to u64 which is 18446744073709551615 - and then we'd try to seek to that position!

my change correctly handles whence + also adds extra safety in the i64 -> u64 cast in the SEEK_SET case

[bavardage]

cc @operutka

[bavardage]

@operutka any chance you can take a look at this one?

[operutka]

Hi @bavardage. Sorry for the delay. It's been a bit hard for me to find some time for this project lately. I'll try to find some time at least for this PR by the end of this week.

[operutka]

I've finally found some time to take a better look at this. Sorry for the delay. I'm glad we're fixing this issue. The only thing that bothers me are those hard-coded constants for SEEK_SET, SEEK_CUR and SEEK_END.

Correct me if I'm wrong, but as far as I know, values of these constants are not standardized. So it could happen (however improbable it is) that there will be a platform using different values for these constants.

I'd prefer transforming the ffw_io_is_avseek_size() function into something like this:

int ffw_io_whence_to_seek_mode(int whence) {
    if (whence & AVSEEK_SIZE) {
        return 0;
    }

    switch (whence) {
        case SEEK_SET: return 1;
        case SEEK_CUR: return 2;
        case SEEK_END: return 3;
        default: return -1;
    }
}

and then in the io_seek() function we'd do something like this:

extern "C" fn io_seek<T>(opaque: *mut c_void, offset: i64, whence: c_int) -> i64
where
    T: Seek,
{
    // helper function
    fn inner(input: &mut T, offset: i64, mode: c_int) -> io::Result<i64> {
        // 0 indicates that the AVSEEK_SIZE flag was set and we should return the current position
        if mode == 0 {
            return get_seekable_length(input)?
                .try_into()
                .map_err(|_| io::Error::new(io::ErrorKind::FileTooLarge, "out of range"));
        }
        
        let pos = match mode {
            // 1 means seek relative to the beginning
            1 => {
                u64::try_from(offset)
                    .map(SeekFrom::Start)
                    .map_err(|_| io::Error::new(io::ErrorKind::InvalidInput, "invalid offset"))?;
            }
            // 2 means seek relative to the current position
            2 => SeekFrom::Current(offset),
            // 3 means seek relative to the end position
            3 => SeekFrom::End(offset),
            _ => return Err(io::Error::new(io::ErrorKind::InvalidInput, "invalid whence")),
        };
        
        input
            .seek(pos)?
            .try_into()
            .map_err(|_| io::Error::new(io::ErrorKind::FileTooLarge, "out of range"));
    }

    ...
    
    let mode = unsafe { ffw_io_whence_to_seek_mode(whence) };
    
    match inner(input, offset, mode) {
        Ok(pos) => pos,
        Err(err) => err
            .raw_os_error()
            .map(|code| unsafe { crate::ffw_error_from_posix(code as _) })
            .unwrap_or_else(|| unsafe { crate::ffw_error_unknown() }) as i64,
    }
}

This way we'll have the constants under control and it will work regardless of the actual values of SEEK_SET, SEEK_CUR and SEEK_END used on the target platform.

[bavardage]

Thanks for the review, and sorry for the slow response.

Makes sense to not assume the constant values - I pushed a commit following your suggestion