[Bug 36641] New: valgrind shows an anvalid read in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641

Bug ID: 36641
           Summary: valgrind shows an anvalid read in wininet/tests/http.c
           Product: Wine
           Version: 1.7.19
          Hardware: x86
                OS: Linux
            Status: NEW
          Keywords: download, source, testcase
          Severity: normal
          Priority: P2
         Component: wininet
          Assignee: [e-mail]
          Reporter: [e-mail]
        Depends on: 36637

==32063== Invalid read of size 2
==32063==    at 0x4D3E66E: HTTP_HttpQueryInfoW (http.c:3596)
==32063==    by 0x4D3F1B0: HttpQueryInfoW (http.c:3883)
==32063==    by 0x4D3F402: HttpQueryInfoA (http.c:3944)
==32063==    by 0x495E46B: InternetReadFile_test (http.c:629)
==32063==    by 0x497D687: func_http (http.c:5329)
==32063==    by 0x4994B90: run_test (test.h:584)
==32063==    by 0x4994F7F: main (test.h:654)
==32063==  Address 0x471e21e is 0 bytes after a block of size 822 alloc'd
==32063==    at 0x7BC4C75D: notify_alloc (heap.c:255)
==32063==    by 0x7BC50FA1: RtlAllocateHeap (heap.c:1716)
==32063==    by 0x4D35000: heap_alloc (internet.h:116)
==32063==    by 0x4D3FD2F: HTTP_build_req (http.c:4170)
==32063==    by 0x4D36905: build_response_header (http.c:732)
==32063==    by 0x4D3E5A4: HTTP_HttpQueryInfoW (http.c:3576)
==32063==    by 0x4D3F1B0: HttpQueryInfoW (http.c:3883)
==32063==    by 0x4D3F402: HttpQueryInfoA (http.c:3944)
==32063==    by 0x495E46B: InternetReadFile_test (http.c:629)
==32063==    by 0x497D687: func_http (http.c:5329)
==32063==    by 0x4994B90: run_test (test.h:584)
==32063==    by 0x4994F7F: main (test.h:654)
==32063==

==32063== Invalid read of size 4
==32063==    at 0x7BC4D939: HEAP_CreateFreeBlock (heap.c:591)
==32063==    by 0x7BC4DC60: HEAP_ShrinkBlock (heap.c:694)
==32063==    by 0x7BC50F6D: RtlAllocateHeap (heap.c:1713)
==32063==    by 0x4D35000: heap_alloc (internet.h:116)
==32063==    by 0x4D3F382: HttpQueryInfoA (http.c:3934)
==32063==    by 0x495E5B7: InternetReadFile_test (http.c:639)
==32063==    by 0x497D6B3: func_http (http.c:5331)
==32063==    by 0x4994B90: run_test (test.h:584)
==32063==    by 0x4994F7F: main (test.h:654)
==32063==  Address 0x4760000 is not stack'd, malloc'd or (recently) free'd
==32063==

==32063== Thread 4:
==32063== Invalid write of size 4
==32063==    at 0x7BC93356: NtQuerySystemTime (time.c:467)
==32063==    by 0x7BC8A8FB: NtDelayExecution (sync.c:940)
==32063==    by 0x7B876351: SleepEx (sync.c:108)
==32063==    by 0x7B87630B: Sleep (sync.c:97)
==32063==    by 0x4D358A2: collect_connections_proc (http.c:358)
==32063==    by 0x7BC87137: ??? (signal_i386.c:2571)
==32063==    by 0x7BC87180: call_thread_func (signal_i386.c:2630)
==32063==    by 0x7BC87115: ??? (signal_i386.c:2571)
==32063==    by 0x7BC8E560: start_thread (thread.c:428)
==32063==    by 0x4EA7BD89: start_thread (in /usr/lib/libpthread-2.18.so)
==32063==    by 0x4E95CA0D: clone (in /usr/lib/libc-2.18.so)
==32063==  Address 0x658e8bc is on thread 1's stack
==32063==

==32063== Invalid read of size 4
==32063==    at 0x7BC1EB70: __x86.get_pc_thunk.bx (in
/home/austin/wine-valgrind/dlls/ntdll/ntdll.dll.so)
==32063==    by 0x7BC8A8FB: NtDelayExecution (sync.c:940)
==32063==    by 0x7B876351: SleepEx (sync.c:108)
==32063==    by 0x7B87630B: Sleep (sync.c:97)
==32063==    by 0x4D358A2: collect_connections_proc (http.c:358)
==32063==    by 0x7BC87137: ??? (signal_i386.c:2571)
==32063==    by 0x7BC87180: call_thread_func (signal_i386.c:2630)
==32063==    by 0x7BC87115: ??? (signal_i386.c:2571)
==32063==    by 0x7BC8E560: start_thread (thread.c:428)
==32063==    by 0x4EA7BD89: start_thread (in /usr/lib/libpthread-2.18.so)
==32063==    by 0x4E95CA0D: clone (in /usr/lib/libc-2.18.so)
==32063==  Address 0x658e8bc is on thread 1's stack
==32063==

etc.

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
            Summary|valgrind shows an anvalid   |valgrind shows several
                   |read in                     |invalid reads in
                   |wininet/tests/http.c        |wininet/tests/http.c

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |valgrind

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

http://bugs.winehq.org/show_bug.cgi?id=36641

Sebastian Lackner  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

--- Comment #1 from Sebastian Lackner  ---
Created attachment 48863
  --> http://bugs.winehq.org/attachment.cgi?id=48863
wininet: Fix invalid memory access in HTTP_QUERY_RAW_HEADERS.

Could you please test this patch? (for the first one)

I'm wondering about two things:

* Why didn't Valgrind complain about the 'for (...)' loop? 'len' is in bytes,
so the old code should be wrong, but Valgrind doesn't notice it.

* As the memcpy(...) is now 2 bytes smaller, we only have one '\0' bytes at the
end, but not two as the MSDN says. When changing the conditions for
ERROR_INSUFFICIENT_BUFFER above then the wine test suite immediately complains
about test failures. Does Windows maybe also write over the end of the
user-provided buffer?

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641

--- Comment #2 from Austin English  ---
(In reply to Sebastian Lackner from comment #1)
> Created attachment 48863 [details]
> wininet: Fix invalid memory access in HTTP_QUERY_RAW_HEADERS.
>
> Could you please test this patch? (for the first one)

works here, thanks.

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641

Sebastian Lackner  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[e-mail]

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641

--- Comment #3 from Sebastian Lackner  ---
Bruno Jesus wrote some tests to verify how these functions behave on Windows,
and submitted a patch including some tests:

http://source.winehq.org/patches/data/105231

This is just the first part, it will also be necessary to write tests for the
*W version of the HttpQueryInfo function.

These tests seem to confirm that Windows really writes over the end of the
user-supplied buffer in some cases (and we probably have to fix the tests
instead of the implementation). Its probably better to gather some more test
results before fixing this issue, otherwise this might cause regressions in
applications, that depend on this broken Windows behaviour.

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641

Austin English  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
      Fixed by SHA1|                            |8fd44a3d2b7438fbf1e7fcfa748
                   |                            |2147999ff4851
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Austin English  ---
https://source.winehq.org/git/wine.git/commitdiff/8fd44a3d2b7438fbf1e7fcfa74...

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641

Alexandre Julliard  changed:

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |CLOSED

--- Comment #5 from Alexandre Julliard  ---
Closing bugs fixed in 1.7.22.

[Bug 36641] valgrind shows several invalid reads in wininet/tests/http.c

https://bugs.winehq.org/show_bug.cgi?id=36641
Bug 36641 depends on bug 36637, which changed state.

Bug 36637 Summary: wininet/tests/http.c crashes under valgrind
https://bugs.winehq.org/show_bug.cgi?id=36637

What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|---                         |FIXED