Skip to content
This repository has been archived by the owner on Feb 14, 2023. It is now read-only.

Multipart.foreach_entry() panics on unseen keys, is it expected? #114

Closed
liufuyang opened this issue Nov 21, 2018 · 9 comments · Fixed by #115
Closed

Multipart.foreach_entry() panics on unseen keys, is it expected? #114

liufuyang opened this issue Nov 21, 2018 · 9 comments · Fixed by #115

Comments

@liufuyang
Copy link

liufuyang commented Nov 21, 2018 •
edited
Loading

Hi there

I am trying to use multipart in the project Tide with building an example like in this PR:
http-rs/tide#47

If you read the comments or take a look at the examples/multipart-form/main.rs file, you could see I tried to use foreach_entry method and make the lambda does nothing (only println) on a key that is not specified in the code.

It works with multipart payload with specified keys, however it panics if a key is not specified and giving error message like Unable to iterate multipart?: Custom { kind: InvalidData, error: TooLarge }

Do users are request to at least run explicitly entry.data.read_to_end() once in they function passed to foreach_entry() method? Or is this a buggy behavior that can be fixed?

Thank you :)
@abonander
Copy link
Owner

abonander commented Nov 21, 2018 •
edited
Loading

Yes, it's a bug. Every call to .read_entry() (used by foreach_entry()) should discard the previous field's data.

@liufuyang
Copy link
Author

Is it easy to fix? Perhaps I can try help or you've got time to handle it? I am new to rust and would love some simple tasks to learn more about it :)

@liufuyang liufuyang mentioned this issue Nov 21, 2018
Merged
@abonander
Copy link
Owner

This unfortunately is going to require some debugging.

That error is being returned from here: https://github.com/abonander/multipart/blob/master/src/server/field.rs#L77

But the problem isn't with that code. The buffer isn't being emptied so it can't read the headers. The method responsible for emptying the buffer is BoundaryReader::consume_boundary() here: https://github.com/abonander/multipart/blob/master/src/server/boundary.rs#L114

But what we need is the raw request body that's triggering this behavior. If it doesn't contain any confidential data, do you mind posting it? Otherwise you should be able to replace the contents and strings in the headers with arbitrary bytes as long as the length is the same.

@liufuyang
Copy link
Author

@abonander I see.

Basically I noticed this problem while developing an example code for the tide project:
http-rs/tide#47

You can see the error by:

git clone [email protected]:rust-net-web/tide.git
cd tide

# remove line 54-55 in examples/multipart-form/main.rs

cargo run --example multipart-form

curl -X POST http://localhost:7878/upload_file -H 'content-type: multipart/form-data' -F key1=v1, -F key2=v2,  -F key3=v3

Lines to remove to see the error:
https://github.com/rust-net-web/tide/blob/master/examples/multipart-form/main.rs#L54-L55

Let me know if this can help you see the issue on your side.

@abonander
Copy link
Owner

So what I need in this context is the request body that curl is sending. You can get that by adding the --trace flag to the command as shown here: https://curl.haxx.se/docs/manpage.html#--trace

Then attach the resulting file to a Github comment so I can inspect it.

@liufuyang
Copy link
Author

Aha, I see, that one I can do easily, here is the output of --trace:

== Info:   Trying ::1...
== Info: TCP_NODELAY set
== Info: Connection failed
== Info: connect to ::1 port 7878 failed: Connection refused
== Info:   Trying 127.0.0.1...
== Info: TCP_NODELAY set
== Info: Connected to localhost (127.0.0.1) port 7878 (#0)
=> Send header, 219 bytes (0xdb)
0000: 50 4f 53 54 20 2f 75 70 6c 6f 61 64 5f 66 69 6c POST /upload_fil
0010: 65 20 48 54 54 50 2f 31 2e 31 0d 0a 48 6f 73 74 e HTTP/1.1..Host
0020: 3a 20 6c 6f 63 61 6c 68 6f 73 74 3a 37 38 37 38 : localhost:7878
0030: 0d 0a 55 73 65 72 2d 41 67 65 6e 74 3a 20 63 75 ..User-Agent: cu
0040: 72 6c 2f 37 2e 35 34 2e 30 0d 0a 41 63 63 65 70 rl/7.54.0..Accep
0050: 74 3a 20 2a 2f 2a 0d 0a 43 6f 6e 74 65 6e 74 2d t: */*..Content-
0060: 4c 65 6e 67 74 68 3a 20 33 33 33 0d 0a 45 78 70 Length: 333..Exp
0070: 65 63 74 3a 20 31 30 30 2d 63 6f 6e 74 69 6e 75 ect: 100-continu
0080: 65 0d 0a 63 6f 6e 74 65 6e 74 2d 74 79 70 65 3a e..content-type:
0090: 20 6d 75 6c 74 69 70 61 72 74 2f 66 6f 72 6d 2d  multipart/form-
00a0: 64 61 74 61 3b 20 62 6f 75 6e 64 61 72 79 3d 2d data; boundary=-
00b0: 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d ----------------
00c0: 2d 2d 2d 2d 2d 2d 2d 63 36 31 36 65 35 66 64 65 -------c616e5fde
00d0: 64 39 36 61 33 63 37 0d 0a 0d 0a                d96a3c7....
<= Recv header, 23 bytes (0x17)
0000: 48 54 54 50 2f 31 2e 31 20 31 30 30 20 43 6f 6e HTTP/1.1 100 Con
0010: 74 69 6e 75 65 0d 0a                            tinue..
=> Send data, 333 bytes (0x14d)
0000: 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d ----------------
0010: 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 63 36 31 36 65 35 ----------c616e5
0020: 66 64 65 64 39 36 61 33 63 37 0d 0a 43 6f 6e 74 fded96a3c7..Cont
0030: 65 6e 74 2d 44 69 73 70 6f 73 69 74 69 6f 6e 3a ent-Disposition:
0040: 20 66 6f 72 6d 2d 64 61 74 61 3b 20 6e 61 6d 65  form-data; name
0050: 3d 22 6b 65 79 31 22 0d 0a 0d 0a 76 31 2c 0d 0a ="key1"....v1,..
0060: 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d ----------------
0070: 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 63 36 31 36 65 35 ----------c616e5
0080: 66 64 65 64 39 36 61 33 63 37 0d 0a 43 6f 6e 74 fded96a3c7..Cont
0090: 65 6e 74 2d 44 69 73 70 6f 73 69 74 69 6f 6e 3a ent-Disposition:
00a0: 20 66 6f 72 6d 2d 64 61 74 61 3b 20 6e 61 6d 65  form-data; name
00b0: 3d 22 6b 65 79 32 22 0d 0a 0d 0a 76 32 2c 0d 0a ="key2"....v2,..
00c0: 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d ----------------
00d0: 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 63 36 31 36 65 35 ----------c616e5
00e0: 66 64 65 64 39 36 61 33 63 37 0d 0a 43 6f 6e 74 fded96a3c7..Cont
00f0: 65 6e 74 2d 44 69 73 70 6f 73 69 74 69 6f 6e 3a ent-Disposition:
0100: 20 66 6f 72 6d 2d 64 61 74 61 3b 20 6e 61 6d 65  form-data; name
0110: 3d 22 6b 65 79 33 22 0d 0a 0d 0a 76 33 0d 0a 2d ="key3"....v3..-
0120: 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d 2d ----------------
0130: 2d 2d 2d 2d 2d 2d 2d 2d 2d 63 36 31 36 65 35 66 ---------c616e5f
0140: 64 65 64 39 36 61 33 63 37 2d 2d 0d 0a          ded96a3c7--..
== Info: Empty reply from server
== Info: Connection #0 to host localhost left intact

abonander added a commit that referenced this issue Nov 27, 2018
@abonander
add repro for #114, some debug asserts to narrow it down
fb1ca3c
@abonander
Copy link
Owner

I've added a reproduction test for this issue to the issue_114 branch along with some debug asserts to narrow down the location of the bug. It appears to be a problem with calculating consume_amt in this block: https://github.com/abonander/multipart/blob/issue_114/src/server/boundary.rs#L135-L175

The assert that triggers right now is the one in the match block at the end so it's somewhere between that and the other debug asserts. I think consume_amt is ending up too low (not consuming the entire boundary) but I have more debugging to do.

@liufuyang
Copy link
Author

Interesting. Thanks for trying to solve it @abonander 😄

abonander added a commit that referenced this issue Nov 28, 2018
@abonander
add fixes and regression tests for #114
fa87701
@abonander abonander mentioned this issue Nov 28, 2018
Merged
@abonander
Copy link
Owner

Fix published as 0.15.4.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix skipped fields not being completely consumed abonander/multipart
2 participants
@liufuyang @abonander