HEAD requests which return a content-length incorrectly Err #321

Firstyear · 2021-08-24T04:42:58Z

Some urls will return a content-length during head requests. Surf incorrectly assumes that this means there is a body present and will error:

thread 'main' panicked at 'Should Succeed!: ResponseBodyError(None): unknown error', src/main.rs:9:28
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The following reproducer can show this behaviour:

[package]
name = "surf-cl-repro"
version = "0.1.0"
edition = "2018"
[dependencies]
surf = "2.2"
url = "2"
[dependencies.async-std]
version = "1.7.0"
features = ["attributes"]

use url::Url;

#[async_std::main]
async fn main() {
    let client = surf::client().with(surf::middleware::Redirect::new(2));
    let url = Url::parse("http://download.opensuse.org/update/tumbleweed/repodata/repomd.xml")
        .expect("invalid url");
    let req = surf::head(url);
    client.send(req).await.expect("Should Succeed!");
}

Expected Results: Surf should allow head requests to proceed even if a content-length is returned.

The text was updated successfully, but these errors were encountered:

Fishrock123 · 2021-08-24T18:02:34Z

Can you run with RUST_BACKTRACE=1? I suspect this is an async-h1 issue.

yoshuawuyts · 2021-08-24T18:17:32Z

The documentation for HTTP HEAD explicitly call out Content-Length as an example of a request which should succeed. This is definitely a bug.

For example, if a URL might produce a large download, a HEAD request could read its Content-Length header to check the filesize without actually downloading the file.

Fishrock123 · 2021-08-24T18:38:58Z

Oh. I think this is a libcurl / isahc problem, it seems familiar.

@Firstyear try using this in your Cargo.toml:

surf = { version = "2.3", default-features = false, features = ["h1-client", "encoding"] }

Firstyear · 2021-08-25T01:14:47Z

RUST_BACKTRACE=1 for @Fishrock123 (no changes to features)

thread 'main' panicked at 'Should Succeed!: ResponseBodyError(None): unknown error', src/main.rs:9:28
stack backtrace:
   0: rust_begin_unwind
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/panicking.rs:493:5
   1: core::panicking::panic_fmt
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/panicking.rs:92:14
   2: core::result::unwrap_failed
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/result.rs:1355:5
   3: core::result::Result<T,E>::expect
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/result.rs:997:23
   4: surf_cl_repro::main::main::{{closure}}
             at ./src/main.rs:9:5
   5: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/future/mod.rs:80:19
   6: surf_cl_repro::main::{{closure}}
             at ./src/main.rs:3:1
   7: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/future/mod.rs:80:19
   8: <async_std::task::builder::SupportTaskLocals<F> as core::future::future::Future>::poll::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:199:17
   9: async_std::task::task_locals_wrapper::TaskLocalsWrapper::set_current::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/task_locals_wrapper.rs:60:13
  10: std::thread::local::LocalKey<T>::try_with
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:376:16
  11: std::thread::local::LocalKey<T>::with
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:352:9
  12: async_std::task::task_locals_wrapper::TaskLocalsWrapper::set_current
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/task_locals_wrapper.rs:55:9
  13: <async_std::task::builder::SupportTaskLocals<F> as core::future::future::Future>::poll
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:197:13
  14: <futures_lite::future::Or<F1,F2> as core::future::future::Future>::poll
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/futures-lite-1.12.0/src/future.rs:526:33
  15: async_executor::Executor::run::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-executor-1.4.1/src/lib.rs:242:9
  16: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/future/mod.rs:80:19
  17: async_executor::LocalExecutor::run::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-executor-1.4.1/src/lib.rs:447:9
  18: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/future/mod.rs:80:19
  19: async_io::driver::block_on
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-io-1.6.0/src/driver.rs:142:33
  20: async_global_executor::reactor::block_on::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-global-executor-2.0.2/src/reactor.rs:3:18
  21: async_global_executor::reactor::block_on
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-global-executor-2.0.2/src/reactor.rs:12:5
  22: async_global_executor::executor::block_on::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-global-executor-2.0.2/src/executor.rs:26:36
  23: std::thread::local::LocalKey<T>::try_with
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:376:16
  24: std::thread::local::LocalKey<T>::with
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:352:9
  25: async_global_executor::executor::block_on
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-global-executor-2.0.2/src/executor.rs:26:5
  26: async_std::task::builder::Builder::blocking::{{closure}}::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:171:25
  27: async_std::task::task_locals_wrapper::TaskLocalsWrapper::set_current::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/task_locals_wrapper.rs:60:13
  28: std::thread::local::LocalKey<T>::try_with
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:376:16
  29: std::thread::local::LocalKey<T>::with
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:352:9
  30: async_std::task::task_locals_wrapper::TaskLocalsWrapper::set_current
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/task_locals_wrapper.rs:55:9
  31: async_std::task::builder::Builder::blocking::{{closure}}
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:168:17
  32: std::thread::local::LocalKey<T>::try_with
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:376:16
  33: std::thread::local::LocalKey<T>::with
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/std/src/thread/local.rs:352:9
  34: async_std::task::builder::Builder::blocking
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/builder.rs:161:9
  35: async_std::task::block_on::block_on
             at /home/william/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.9.0/src/task/block_on.rs:33:5
  36: surf_cl_repro::main
             at ./src/main.rs:3:1
  37: core::ops::function::FnOnce::call_once
             at /rustc/53cb7b09b00cbea8754ffb78e7e3cb521cb8af4b/library/core/src/ops/function.rs:227:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

With surf = { version = "2.3", default-features = false, features = ["h1-client", "encoding"] } this succeeds so I think you are correct about it being a isahc issue :)

Fishrock123 · 2021-08-25T16:31:09Z

We may want to report that to the isahc repo, but note that we run 0.9 instead of 1.x

Firstyear · 2021-08-26T01:17:21Z

Is there a major set of changes to move to 1.x first? And did you want me to report that issue or can we proxy it over somehow?

Fishrock123 · 2021-08-26T18:06:27Z

@Firstyear We probably need to introduce versioned feature flags ...

Please report this to Isahc, yeah

Firstyear · 2021-08-26T23:42:02Z

sagebind/isahc#341

sagebind · 2021-08-28T03:48:12Z

Actually this looks the same as #218 perhaps?

I dug into this example program with LLDB and figured out the cause; it is kind of a Surf bug, and kind of an Isahc bug. Let's start with the former.

Surf is constructing a request for Isahc to send, and gives it a request body to upload of length 0. Note that this is not the same as the absence of a request body! The way I interpret the spec (RFC 7230, Section 3.3), there is a clear semantic difference between a zero-length payload and the absence of a payload. In particular:

The presence of a message body in a request is signaled by a Content-Length or Transfer-Encoding header field. Request message framing is independent of method semantics, even if the method does not define any use for a message body.

Or maybe even more clear:

[...] All other responses do include a message body, although the body might be of zero length.

This implies that a message with a Content-Length: 0 request header is semantically different than a request with no such header and no body. Isahc is following this distinction here in that isahc::Body::from_reader_sized(body, 0) is not the same as isahc::Body::empty()! Surf is always doing the former, even for HEAD requests, here: https://github.com/http-rs/http-client/blob/3c0ed9a2f350749f48f579867139dd9e3afdf182/src/isahc.rs#L53-L56.

Maybe Isahc should have represented bodies as Option<Body> to be more semantically clear, but that ship has sailed for now.

Now this is a relatively minor bug, but unfortunately it triggers an Isahc bug described in sagebind/isahc#342, in which Isahc fails trying to read a response body when the server sends a response with a Content-Length header to such a HEAD request containing a request body. For now I'll work on fixing the Isahc bug. If you'd really like to avoid upgrading to 1.x for right now I can backport the bugfix to the 0.9.x series too.

Fishrock123 · 2021-08-30T17:17:50Z

The issue you are describing seems like it's an api shortcoming of how isahc works with the http crate... The problem stems from the interactions of this api: https://docs.rs/http/0.2.4/http/request/struct.Builder.html#method.body

I think I can work around this with some extra code though, for better or worse...

If there is no body then don't construct a zero-lengthed body. Fixes: http-rs/surf#321 Fixes: http-rs/surf#218

Fishrock123 · 2021-08-30T17:26:10Z

Could someone check if this patch fixes the issue? http-rs/http-client#97 (Edit: original patch was wrong, should be good now.)

If there is no body then don't construct a zero-lengthed body. Fixes: http-rs/surf#321 Fixes: http-rs/surf#218

Fishrock123 · 2021-08-30T17:31:44Z

Regarding isahc 0.9 vs 1.x - we ("we" as in who ever did this at the time, who was not really me (i.e. pre 2.x)) messed up and made feature flags on multiple clients which do not pin to their major versions...

curl-client is a bad flag name anyways, and should have been something like curl-isahc09-client, then we could have easily had an curl-isahc1-client, etc. the same absurd problem exists for hyper / tokio, fwiw.

If there is no body then don't construct a zero-lengthed body. Fixes: http-rs/surf#321 Fixes: http-rs/surf#218

Firstyear · 2021-08-30T22:32:45Z

Tested with:

[patch.crates-io]
http-client = { git = "https://github.com/Fishrock123/http-client.git", branch = "fix-isahc-head-zero-size" }

Can confirm that it works now :)

Thank you!

Firstyear · 2021-09-14T05:33:45Z

There may be another way to trigger this, as I've started to see other calls have this issue now.

[package]
name = "surf-cl-repro"
version = "0.1.0"
edition = "2018"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

surf = "2.3"
url = "2"
tracing-subscriber = "0.2"

[dependencies.async-std]
version = "1.7.0"
features = ["attributes"]

[patch.crates-io]
http-client = { git = "https://github.com/Fishrock123/http-client.git", branch = "fix-isahc-head-zero-size" }

use url::Url;
use tracing_subscriber;

#[async_std::main]
async fn main() {

    tracing_subscriber::fmt::init();

    let client = surf::client().with(surf::middleware::Redirect::new(3));
    let url = Url::parse("http://download.opensuse.org/tumbleweed/iso/openSUSE-Tumbleweed-NET-x86_64-Current.iso")
        .expect("invalid url");
    let req = surf::head(url);
    let res = client.send(req).await.expect("Should Succeed!");
    println!("{:?}", res);
}

Perhaps the fixed branch is incomplete?

Firstyear · 2021-09-14T10:37:30Z

@Fishrock123 I think I've solved it. Looking at http-client/src/isahc.rs I think you need to match on Some(0) | None => because in some cases it does a Some(0) which would still triger the other path.

Firstyear mentioned this issue Aug 26, 2021

head requests with content length fail sagebind/isahc#341

Closed

Fishrock123 added a commit to Fishrock123/http-client that referenced this issue Aug 30, 2021

fix: curl - avoid zero-sized HEAD bodies

c8eeaff

If there is no body then don't construct a zero-lengthed body. Fixes: http-rs/surf#321 Fixes: http-rs/surf#218

Fishrock123 linked a pull request Aug 30, 2021 that will close this issue

fix: curl - avoid zero-sized HEAD bodies http-rs/http-client#97

Open

Fishrock123 added a commit to Fishrock123/http-client that referenced this issue Aug 30, 2021

fix: curl - avoid zero-sized HEAD bodies

44cba08

If there is no body then don't construct a zero-lengthed body. Fixes: http-rs/surf#321 Fixes: http-rs/surf#218

Fishrock123 added a commit to Fishrock123/http-client that referenced this issue Aug 30, 2021

fix: curl - avoid zero-sized HEAD bodies

07b7317

If there is no body then don't construct a zero-lengthed body. Fixes: http-rs/surf#321 Fixes: http-rs/surf#218

Fishrock123 added the bug Something isn't working label Aug 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HEAD requests which return a content-length incorrectly Err #321

HEAD requests which return a content-length incorrectly Err #321

Firstyear commented Aug 24, 2021 •

edited by Fishrock123

Loading

Fishrock123 commented Aug 24, 2021 •

edited

Loading

yoshuawuyts commented Aug 24, 2021

Fishrock123 commented Aug 24, 2021 •

edited

Loading

Firstyear commented Aug 25, 2021

Fishrock123 commented Aug 25, 2021

Firstyear commented Aug 26, 2021

Fishrock123 commented Aug 26, 2021

Firstyear commented Aug 26, 2021

sagebind commented Aug 28, 2021

Fishrock123 commented Aug 30, 2021

Fishrock123 commented Aug 30, 2021 •

edited

Loading

Fishrock123 commented Aug 30, 2021 •

edited

Loading

Firstyear commented Aug 30, 2021

Firstyear commented Sep 14, 2021

Firstyear commented Sep 14, 2021

HEAD requests which return a content-length incorrectly Err #321

HEAD requests which return a content-length incorrectly Err #321

Comments

Firstyear commented Aug 24, 2021 • edited by Fishrock123 Loading

Fishrock123 commented Aug 24, 2021 • edited Loading

yoshuawuyts commented Aug 24, 2021

Fishrock123 commented Aug 24, 2021 • edited Loading

Firstyear commented Aug 25, 2021

Fishrock123 commented Aug 25, 2021

Firstyear commented Aug 26, 2021

Fishrock123 commented Aug 26, 2021

Firstyear commented Aug 26, 2021

sagebind commented Aug 28, 2021

Fishrock123 commented Aug 30, 2021

Fishrock123 commented Aug 30, 2021 • edited Loading

Fishrock123 commented Aug 30, 2021 • edited Loading

Firstyear commented Aug 30, 2021

Firstyear commented Sep 14, 2021

Firstyear commented Sep 14, 2021

Firstyear commented Aug 24, 2021 •

edited by Fishrock123

Loading

Fishrock123 commented Aug 24, 2021 •

edited

Loading

Fishrock123 commented Aug 24, 2021 •

edited

Loading

Fishrock123 commented Aug 30, 2021 •

edited

Loading

Fishrock123 commented Aug 30, 2021 •

edited

Loading