bug: Reader's seek operation incorrectly panic when the parameter is not valid

[dqhl76]

For the seek operation of OpenDAL's reader, if seek to a negative or overflowing position, it will cause panic unexpectedly.

How to reproduce?

let r = op.reader("path/to/file")
r.seek(SeekFrom::Current(-1024))

Error occurred: Unexpected (permanent) at Reader::seek => invalid seek to a negative or overflowing position

What we expected?

It will not cause panic and return the error in the seek's result

---

related discussion: https://discord.com/channels/1081052318650339399/1122785258501308497/1129261665146388490

[Xuanwo]

It will not cause panic and return the error in the seek's result

Yep! We should make sure all our internal logic hanlde those correctly. Would you like to go deep and try fix it?

[dqhl76]

It will not cause panic and return the error in the seek's result

Yep! We should make sure all our internal logic hanlde those correctly. Would you like to go deep and try fix it?

I am not familar with that but I would like to have a try

[Xuanwo]

I am not familar with that but I would like to have a try

I'm willing to provide help! You can debug in this way:

• Add a behavior test for this case
• running our test the following command (I think test memory, fs and s3 is enough):

  RUST_BACKTRACE=1 RUST_LOG=debug cargo test memory -- --nocapture

• Post the backtrace here, we can find the root cause of this panic

[dqhl76]

• Post the backtrace here, we can find the root cause of this panic
  I add a behaviour test

/// seek a negative position
pub async fn test_reader_seek_invalid(op: Operator) -> Result<()> {
    let path = uuid::Uuid::new_v4().to_string();
    debug!("Generate a random file: {}", &path);
    let (content, size) = gen_bytes();

    op.write(&path, content.clone())
        .await
        .expect("write must succeed");

    let mut r = op.reader(&path).await?;
    r.seek(std::io::SeekFrom::Current(-1024)).await?;
    
    op.delete(&path).await.expect("delete must succeed");
    Ok(())
}

The log is

  2023-07-14T06:37:19.152901Z DEBUG opendal::services: service=memory operation=read path=bfae56eb-ae99-4ca6-b000-4bdad69c85e1 range=0- -> got reader
    at core/src/layers/logging.rs:273

  2023-07-14T06:37:19.152933Z ERROR opendal::services: service=memory operation=Reader::seek path=bfae56eb-ae99-4ca6-b000-4bdad69c85e1 read=0 -> data read failed: Unexpected (permanent) at Reader::seek => invalid seek to a negative or overflowing position

Context:
    service: memory
    path: bfae56eb-ae99-4ca6-b000-4bdad69c85e1

    at core/src/layers/logging.rs:1108

  2023-07-14T06:37:19.153098Z DEBUG opendal::services: service=memory operation=read path=bfae56eb-ae99-4ca6-b000-4bdad69c85e1 read=0 -> data read finished
    at core/src/layers/logging.rs:1035

[Xuanwo]

Thanks for the find! We can add a new error code InvalidInput for that.

When we got an invalid input like SeekFrom::Current(-1024), we should return InvalidInput instead of Unexpected so callers can catch them.

Would you like to submit a PR for this change?

[Xuanwo]

By the way, it's not a panic. Maybe there are other problems (like in fs or s3).

[dqhl76]

I have just one quesion.

Why this Err cannot be catched by something like this https://gist.github.com/dqhl76/76cdb6364339ced8867db8eafdd7b7c2

Err(Error::new(
                ErrorKind::Unexpected,
                "invalid seek to a negative or overflowing position",
            ))

[dqhl76]

Would you like to submit a PR for this change?

Sure, I will do that.

[Xuanwo]

Why this Err cannot be catched by something like this https://gist.github.com/dqhl76/76cdb6364339ced8867db8eafdd7b7c2

If the error is thrown due to panic, there is no way for us to catch it. That's why I advised #2636 (comment)

If it's a normal error returned (like in this case), we can handle it.