[Bug 37023] New: winhttp: HEAD-requests are not handled correctly
http://bugs.winehq.org/show_bug.cgi?id=37023 Bug ID: 37023 Summary: winhttp: HEAD-requests are not handled correctly Product: Wine Version: 1.7.23 Hardware: x86 OS: Linux Status: UNCONFIRMED Severity: major Priority: P2 Component: winhttp Assignee: wine-bugs(a)winehq.org Reporter: anduchs(a)gmail.com Created attachment 49194 --> http://bugs.winehq.org/attachment.cgi?id=49194 Test case for this issue When opening a HEAD-request, winhttp tries to read response-contents even though there is none coming. #29715 mentioned the same problem as fixed, but it does still happen / happen again. Regression or new case ? Attached are a testcase and a patch. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #1 from Andreas <anduchs(a)gmail.com> --- Created attachment 49195 --> http://bugs.winehq.org/attachment.cgi?id=49195 Patch to fix HEAD-requests in winhttp -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
http://bugs.winehq.org/show_bug.cgi?id=37023 Austin English <austinenglish(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |download, patch, testcase -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #2 from Andreas <anduchs(a)gmail.com> --- Created attachment 49400 --> https://bugs.winehq.org/attachment.cgi?id=49400 Wine-TestSuit: Add test for winhttp HEAD requests Unfortunately based on timing, but that seems to be the only way to detect this error. Or does anyone have a better idea on how to detect a timeout-case inside winhttp ? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 Mike Ellery <mellery(a)gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mellery(a)gmail.com -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 Sebastian Lackner <sebastian(a)fds-team.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sebastian(a)fds-team.de -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #3 from Sebastian Lackner <sebastian(a)fds-team.de> --- I don't see a big problem with timing based tests (especially when you send first the patch to fix it), but you should probably use something a bit more easy like GetTickCount(). Moreover, the timeout shouldn't be too high since this is only a localhost connection, and never expected to take several seconds. I am not sure about the fix, a better method would be to set request->content_length to zero when no answer is expected, like: --- snip --- diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index d497d7a..c07e663 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1806,2 +1806,3 @@ static DWORD set_content_length( request_t *request ) { + static const WCHAR headW[] = {'H','E','A','D',0}; WCHAR encoding[20]; @@ -1823,2 +1824,10 @@ static DWORD set_content_length( request_t *request ) } + + /* For a HEAD request we never expect any content */ + if (request->verb && !strcmpiW(request->verb, headW)) + { + request->content_length = 0; + request->read_chunked = FALSE; + } + request->content_read = 0; --- snip --- The main issue I see is that the tests do not even work on Windows: http://newtestbot.winehq.org/JobDetails.pl?Key=10229 Does this make the bug invalid, or does your testcase just not properly reproduce the issue? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #4 from Andreas Fuchs <anduchs(a)gmail.com> ---
Does this make the bug invalid, or does your testcase just not properly reproduce the issue?
I guess my testcase does not really work. I went through some iterations on the similar issue in https://bugs.winehq.org/show_bug.cgi?id=36937 . So I'll have to get this one up to par... Attachment 1 "Test case for this issue" should however still yield different behaviours right ? I hope to go through the stuff again soon. Since there is still a blocker on Marvel Heros that I cannot trace down even after these two patches, I'm lacking interest a bit... ;-) -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #5 from Andreas Fuchs <anduchs(a)gmail.com> ---
I am not sure about the fix, a better method would be to set request->content_length to zero when no answer is expected, like:
--- snip --- diff --git a/dlls/winhttp/request.c b/dlls/winhttp/request.c index d497d7a..c07e663 100644 --- a/dlls/winhttp/request.c +++ b/dlls/winhttp/request.c @@ -1806,2 +1806,3 @@ static DWORD set_content_length( request_t *request ) { + static const WCHAR headW[] = {'H','E','A','D',0}; WCHAR encoding[20]; @@ -1823,2 +1824,10 @@ static DWORD set_content_length( request_t *request ) } + + /* For a HEAD request we never expect any content */ + if (request->verb && !strcmpiW(request->verb, headW)) + { + request->content_length = 0; + request->read_chunked = FALSE; + } + request->content_read = 0; --- snip ---
Looking at this patch I'm sceptical. One of the points about a head-request is, that the server's answer will contain a content_length field. So I'm not sure, what wine's winhttp returns when being asked for the content_length. I _guess_ WinHttpQueryHeaders() is not a problem. Are there other ways that the content_length-field will be accessed by the application ? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #6 from Sebastian Lackner <sebastian(a)fds-team.de> --- (In reply to Andreas Fuchs from comment #4)
Attachment 1 [details] "Test case for this issue" should however still yield different behaviours right ?
Yes, the behaviour is different. Nevertheless, as it seems to fail completely on Windows, I normally wouldn't expect that any app tries to use it - except its an issue somewhere in your testcase. (In reply to Andreas Fuchs from comment #5)
Looking at this patch I'm sceptical. One of the points about a head-request is, that the server's answer will contain a content_length field. So I'm not sure, what wine's winhttp returns when being asked for the content_length. I _guess_ WinHttpQueryHeaders() is not a problem. Are there other ways that the content_length-field will be accessed by the application ?
As far as I know the content_length field is only used for receiving the data, when the app tries to check the header fields the corresponding entry from an internal list is used instead. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 Anastasius Focht <focht(a)gmx.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |NEW URL| |http://cdn.marvelheroes.com | |/marvelheroes/binaries/marv | |elheroesinstaller.exe CC| |focht(a)gmx.net Summary|winhttp: HEAD-requests are |winhttp: HEAD-requests are |not handled correctly |not handled correctly | |(Marvel Heroes 2015 | |launcher) Ever confirmed|0 |1 Severity|major |normal --- Comment #7 from Anastasius Focht <focht(a)gmx.net> --- Hello folks, confirming, still present. Lowering severity since there is only one application in the wild reported, suffering from this yet. Retested with Marvel Heroes 2015 and Wine 1.7.34 ('winetricks -q winhttp' obviously works around). $ sha1sum marvelheroesinstaller.exe 029aac847631e768a3a99e55b5202e4ad9748a42 marvelheroesinstaller.exe $ du -sh marvelheroesinstaller.exe 64M marvelheroesinstaller.exe $ wine --version wine-1.7.34 Regards -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #8 from Hans Leidekker <hans(a)meelstraat.net> --- Created attachment 50442 --> https://bugs.winehq.org/attachment.cgi?id=50442 patch Can you try this patch? -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #9 from Anastasius Focht <focht(a)gmx.net> --- Hello Hans, the patch works on top of wine-1.7.34, thanks :) The bootstrapper/launcher can now download metadata, full client and all incremental patches (13+ GiB) from a clean 32-bit WINEPREFIX without any overrides. Regards -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 Anastasius Focht <focht(a)gmx.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |michael.m.hensley(a)gmail.com --- Comment #10 from Anastasius Focht <focht(a)gmx.net> --- *** Bug 37682 has been marked as a duplicate of this bug. *** -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 --- Comment #11 from Hans Leidekker <hans(a)meelstraat.net> --- Should be fixed by 27ba8c834326bb526fc57148feb6d83883c7a7c6. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 Anastasius Focht <focht(a)gmx.net> changed: What |Removed |Added ---------------------------------------------------------------------------- Fixed by SHA1| |27ba8c834326bb526fc57148feb | |6d83883c7a7c6 Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #12 from Anastasius Focht <focht(a)gmx.net> --- Hello folks, this works now. Fixed by commit http://source.winehq.org/git/wine.git/commitdiff/27ba8c834326bb526fc57148feb... Thanks Hans Regards -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
https://bugs.winehq.org/show_bug.cgi?id=37023 Alexandre Julliard <julliard(a)winehq.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |CLOSED --- Comment #13 from Alexandre Julliard <julliard(a)winehq.org> --- Closing bugs fixed in 1.7.35. -- Do not reply to this email, post in Bugzilla using the above URL to reply. You are receiving this mail because: You are watching all bug changes.
participants (1)
-
wine-bugs＠winehq.org