[PATCH 0/1] MR1187: msvcrt: Fix crash due to link err caused by global variable redefine
On Mon Oct 31 14:35:30 2022 +0000, Wu Liande wrote:
_Condition_variable_notify_one and DEFINE_THISCALL_WRAPPER(_Condition_variable_notify_one, 4) are redefined in other dynamic libraries. Therefore, it caused the problem of global symbol coverage, which caused crash. I still don't get it. Could you please describe the problem you're fixing with more details? Do you see a crash in Windows application?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1187#note_12662
On Tue Nov 1 17:57:43 2022 +0000, Piotr Caban wrote:
I still don't get it. Could you please describe the problem you're fixing with more details? Do you see a crash in Windows application? The application loads two different a.so b.so, they define the global variable g_t with the same name. When dynamically linked, only one g_t is recorded in the global symbol table. Caused the address of one of the variables to be overwritten, which caused an unpredictable error. _Condition_variable_notify_one and DEFINE_THISCALL_WRAPPER(\_Condition_variable_notify_one, 4) define two var of same name in different file。
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1187#note_12737
On Wed Nov 2 10:13:47 2022 +0000, Wu Liande wrote:
The application loads two different a.so b.so, they define the global variable g_t with the same name. When dynamically linked, only one g_t is recorded in the global symbol table. Caused the address of one of the variables to be overwritten, which caused an unpredictable error. _Condition_variable_notify_one and DEFINE_THISCALL_WRAPPER(\_Condition_variable_notify_one, 4) define two var of same name in different file。 Sorry but it still doesn't make sense for me. While I understand it might be a problem for Unix application that loads multiple .so files I don't see what's the problem in Wine's case. Do you see the crash in Windows application? What are the steps to reproduce the problem?
FWIW I don't argue about the changes you made. The _Condition_variable_* function pointers should be static. I just don't understand the commit message / the problem you're seeing. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1187#note_12750
From: wuliande <wuliande(a)bytedance.com> --- dlls/msvcp90/msvcp_main.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/dlls/msvcp90/msvcp_main.c b/dlls/msvcp90/msvcp_main.c index 4c9faec9e26..8717384c2c6 100644 --- a/dlls/msvcp90/msvcp_main.c +++ b/dlls/msvcp90/msvcp_main.c @@ -88,10 +88,10 @@ static bool (__thiscall *critical_section_trylock)(critical_section*); static _Condition_variable* (__thiscall *_Condition_variable_ctor)(_Condition_variable*); static void (__thiscall *_Condition_variable_dtor)(_Condition_variable*); static void (__thiscall *_Condition_variable_wait)(_Condition_variable*, critical_section*); -bool (__thiscall *_Condition_variable_wait_for)(_Condition_variable*, +static bool (__thiscall *_Condition_variable_wait_for)(_Condition_variable*, critical_section*, unsigned int); -void (__thiscall *_Condition_variable_notify_one)(_Condition_variable*); -void (__thiscall *_Condition_variable_notify_all)(_Condition_variable*); +static void (__thiscall *_Condition_variable_notify_one)(_Condition_variable*); +static void (__thiscall *_Condition_variable_notify_all)(_Condition_variable*); void cs_init(cs *cs) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/1187
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=125553 Your paranoid android. === debian11 (32 bit report) === d3d9: device.c:4255: Test failed: Expected message 0x7e for window 0, but didn't receive it, i=1.
Hi Liande, Please format the author name of the patch, capitalizing the initialize character of the first name and the last name. And I think the commit message is improvable, please read https://wiki.winehq.org/Submitting_Patches#The_commit_message for more information. I'm not an expert on VC runtime, just trying to point out some generic issues. Thanks for your contribution. :) -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1187#note_12473
On Mon Oct 31 11:16:43 2022 +0000, Jactry Zeng wrote:
Hi Liande, Please format the author name of the patch, capitalizing the initialize character of the first name and the last name. And I think the commit message is improvable, please read https://wiki.winehq.org/Submitting_Patches#The_commit_message for more information. I'm not an expert on VC runtime, just trying to point out some generic issues. Thanks for your contribution. :) The patch looks OK but commit message might be improved. I don't know what crash do you have in mind (is it even possible to see it without modifying wine source?). How about changing it to `msvcp90: Make _Condition_variable_* function pointers static.`?
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1187#note_12486
On Mon Oct 31 11:16:43 2022 +0000, Piotr Caban wrote:
The patch looks OK but commit message might be improved. I don't know what crash do you have in mind (is it even possible to see it without modifying wine source?). How about changing it to `msvcp90: Make _Condition_variable_* function pointers static.`? _Condition_variable_notify_one and DEFINE_THISCALL_WRAPPER(_Condition_variable_notify_one, 4) are redefined in other dynamic libraries. Therefore, it caused the problem of global symbol coverage, which caused crash.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/1187#note_12515
On Wed Nov 2 11:34:34 2022 +0000, Piotr Caban wrote:
Sorry but it still doesn't make sense for me. While I understand it might be a problem for Unix application that loads multiple .so files I don't see what's the problem in Wine's case. Do you see the crash in Windows application? What are the steps to reproduce the problem? FWIW I don't argue about the changes you made. The _Condition_variable_* function pointers should be static. I just don't understand the commit message / the problem you're seeing.
@piotr i will close this commit because of personal reasons .i recommit this after amoment。 -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1187#note_12799
This merge request was closed by Wu Liande. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/1187
participants (5)
-
Jactry Zeng (＠jactry) -
Marvin -
Piotr Caban (＠piotr)
-
wu liande (＠wuliande)
-
wuliande