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

deadlock in Tempfile::read_field with TestRequest #3202

Open
lovasoa opened this issue Nov 23, 2023 · 0 comments
Open

deadlock in Tempfile::read_field with TestRequest #3202

lovasoa opened this issue Nov 23, 2023 · 0 comments
Labels
A-multipart project: actix-multipart needs-investigation

Comments

@lovasoa
Copy link

lovasoa commented Nov 23, 2023

I was creating a test for a case where a client sends an invalid multipart request without a closing boundary:

use actix_multipart::{
    form::{tempfile::TempFile, FieldReader},
    Multipart,
};
use actix_web::{
    dev::{self, ServiceResponse},
    error::ErrorBadRequest,
    web, App, FromRequest, HttpServer, middleware::Logger,
};
use futures::StreamExt;

async fn test(srv_req: dev::ServiceRequest) -> actix_web::Result<dev::ServiceResponse> {
    let (req, mut pl) = srv_req.into_parts();
    let mut multipart = Multipart::from_request(&req, &mut pl).await?;
    let file_field = multipart.next().await.ok_or(ErrorBadRequest("no file"))??;
    let mut limits = actix_multipart::form::Limits::new(1000, 1000);
    let file_obj = TempFile::read_field(&req, file_field, &mut limits).await?;
    let resp = format!(
        "Loaded file {:?} in {:?}",
        file_obj.file_name,
        file_obj.file.path()
    );
    Ok(ServiceResponse::new(
        req,
        actix_web::HttpResponse::Ok().body(resp),
    ))
}

#[actix_web::main]
async fn main() -> std::io::Result<()> {
    env_logger::init();
    let srv = HttpServer::new(|| 
            App::new().service(web::service("/").finish(test))
            .wrap(Logger::default())
        )
        .bind("127.0.0.1:8080")?
        .run();
    srv.await
}

#[actix_web::test]
async fn test_problematic_payload() -> std::io::Result<()> {
    use actix_web::test::TestRequest;

    let testrequest = TestRequest::get()
        .insert_header(("content-type", "multipart/form-data;boundary=xxx"))
        .set_payload(
            "--xxx\r\n\
        Content-Disposition: form-data; name=\"my_uploaded_file\"; filename=\"test.txt\"\r\n\
        \r\n\
        Hello World\r\n\
        ", // no closing boundary
        )
        .to_srv_request();
    let resp = test(testrequest).await.unwrap();
    println!("Response: {:?}", resp.response().body());
    Ok(())
}

Expected Behavior

I would have expected the behavior with TestRequest to be the same as the one with the live server.

Current Behavior

When running the live server and testing it manually with

nc -N localhost 8080 << EOF
POST / HTTP/1.1
Host: localhost:8081
User-Agent: curl/7.81.0
Accept: */*
Content-Length: 452
Content-Type: multipart/form-data; boundary=------------------------bf59f5a0357ff623

--------------------------bf59f5a0357ff623
Content-Disposition: form-data; name="my_file"; filename="hosts"
Content-Type: application/octet-stream


hello world
I do not have a closing boundary
EOF

Nothing deadlocks, and I get a proper response:

HTTP/1.1 400 Bad Request
content-length: 30
content-type: text/plain; charset=utf-8
date: Thu, 23 Nov 2023 20:17:20 GMT

Multipart stream is incomplete

But when running the test with cargo test, it simply never returns.

@robjtede robjtede added needs-investigation A-multipart project: actix-multipart labels Nov 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multipart project: actix-multipart needs-investigation
Projects
None yet
Development

No branches or pull requests

2 participants