Skip to content
Snippets Groups Projects

Draft: msvcrt: Protect setlocale against concurrent accesses.

Closed Bernhard Übelacker requested to merge bernhardu/wine:55467_setlocale into master
6 unresolved threads

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
801 const char* locale = NULL;
802 if (*(int*)arg % 2) locale = "German_Germany.1252";
803 setlocale(LC_NUMERIC, locale);
804 return 0;
805 }
806
807 static void test_setlocale_threads(void)
808 {
809 HANDLE threads[100];
810 for (int i = 0; i < ARRAY_SIZE(threads); i++)
811 {
812 threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)&i, 0, NULL);
813 ok(threads[i] != NULL, "CreateThread failed\n");
814 }
815 WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, INFINITE);
816 }
  • Comment on lines +799 to +816

    The test needs more work:

    • you should pass i as argument (not &i)
    • ARRAY_SIZE(threads) is bigger than MAXIMUM_WAIT_OBJECTS
    • I think the test will be better if only 2 threads are created
  • Thanks for having a look. The test failing manifests just with a crash. I wanted to have the test crash like the application and took a few threads too much. If I reduce the threads to just two I do no longer receive the crash, with 8 not always, with 16 quite reliable (with a 16 thread cpu).

  • Please register or sign in to reply
  • Piotr Caban
    Piotr Caban @piotr started a thread on an outdated change in commit 942e7be9
  • 2034 2034 */
    2035 2035 char* CDECL setlocale(int category, const char* locale)
    2036 2036 {
    2037 thread_data_t *data = msvcrt_get_thread_data();
    2038 pthreadlocinfo locinfo = get_locinfo(), newlocinfo;
    2037 thread_data_t *data;
    2038 pthreadlocinfo locinfo, newlocinfo;
    2039
    2040 _lock_locales();
    • While this change works we should limit the time when _lock_locales lock is held. It can be achieved by acquiring the lock, increasing locale refcounts and releasing the lock (there's grab_locinfo helper that does that).

    • Bernhard Übelacker changed this line in version 2 of the diff

      changed this line in version 2 of the diff

    • I did some testing to reduce the time and found just using the grab_locinfo makes the crash go away, without using any _lock_locales. Is it then not needed? And from looking at the other grab_locinfo uses I did not see how it gets released later, maybe just by the free_locinfo calls?

    • Please register or sign in to reply
  • Bernhard Übelacker added 123 commits

    added 123 commits

    Compare with previous version

  • v2:

    • Pass i by value to the thread function.
    • Use less threads, at least below MAXIMUM_WAIT_OBJECTS.
    • Replace _lock_locales by grab_locinfo.
  • Piotr Caban
    Piotr Caban @piotr started a thread on commit 2076f93e
  • 796 796 }
    797 797 }
    798 798
    799 static DWORD WINAPI test_setlocale_threads_proc(void *arg)
    800 {
    801 const char* locale = NULL;
    802 if ((intptr_t)arg % 2) locale = "German_Germany.1252";
    803 setlocale(LC_NUMERIC, locale);
    • Comment on lines +801 to +803

      It still crashes for me when setlocale(LC_NUMERIC, NULL) calls are removed. It even seems to crash more often.

      Suggested change
      801 const char* locale = NULL;
      802 if ((intptr_t)arg % 2) locale = "German_Germany.1252";
      803 setlocale(LC_NUMERIC, locale);
      801 setlocale(LC_NUMERIC, "German_Germany.1252");
    • Please register or sign in to reply
  • Piotr Caban
    Piotr Caban @piotr started a thread on commit 2076f93e
  • 800 {
    801 const char* locale = NULL;
    802 if ((intptr_t)arg % 2) locale = "German_Germany.1252";
    803 setlocale(LC_NUMERIC, locale);
    804 return 0;
    805 }
    806
    807 static void test_setlocale_threads(void)
    808 {
    809 HANDLE threads[64];
    810 for (intptr_t i = 0; i < ARRAY_SIZE(threads); i++)
    811 {
    812 threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)i, 0, NULL);
    813 ok(threads[i] != NULL, "CreateThread failed\n");
    814 }
    815 WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, INFINITE);
    • Comment on lines +809 to +815
      Suggested change
      809 HANDLE threads[64];
      810 for (intptr_t i = 0; i < ARRAY_SIZE(threads); i++)
      811 {
      812 threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)i, 0, NULL);
      813 ok(threads[i] != NULL, "CreateThread failed\n");
      814 }
      815 WaitForMultipleObjects(ARRAY_SIZE(threads), threads, TRUE, INFINITE);
      809 HANDLE threads[64];
      810 int i;
      811
      812 for (i = 0; i < ARRAY_SIZE(threads); i++)
      813 {
      814 threads[i] = CreateThread(NULL, 0, test_setlocale_threads_proc, (void*)i, 0, NULL);
      815 ok(threads[i] != NULL, "CreateThread failed\n");
      816 }
      817 for (i = 0; i < ARRAY_SIZE(threads); i++)
      818 {
      819 WaitForSingleObject(threads[i], INFINITE);
      820 CloseHandle(threads[i]);
      821 }
    • Please register or sign in to reply
  • Piotr Caban
    Piotr Caban @piotr started a thread on commit 2076f93e
  • 2037 2037 thread_data_t *data = msvcrt_get_thread_data();
    2038 2038 pthreadlocinfo locinfo = get_locinfo(), newlocinfo;
    2039 2039
    2040 grab_locinfo(locinfo);
    2041
  • Piotr Caban
    Piotr Caban @piotr started a thread on commit 2076f93e
  • 2047 2049 return locinfo->lc_category[category].locale;
    2048 2050 }
    2049 2051
    2050 2052 newlocinfo = create_locinfo(category, locale, locinfo);
    • Suggested change
      2052 newlocinfo = create_locinfo(category, locale, locinfo);
      2052 /* Make sure that locinfo is not freed when thread locale is updated by e.g. stricmp(). */
      2053 grab_locinfo(locinfo);
      2054 newlocinfo = create_locinfo(category, locale, locinfo);
      2055 free_locinfo(locinfo);
    • After thinking about it more it would be even better to do something like:

      Suggested change
      2052 newlocinfo = create_locinfo(category, locale, locinfo);
      2052 /* Make sure that locinfo is not updated by e.g. stricmp function */
      2053 locale_flags = data->locale_flags;
      2054 data->locale_flags |= LOCALE_THREAD;
      2055 newlocinfo = create_locinfo(category, locale, locinfo);
      2056 data->locale_flags = locale_flags;

      This way all the helper functions will be called with the same locale.

    • Please register or sign in to reply
  • Piotr Caban mentioned in merge request !4816 (closed)

    mentioned in merge request !4816 (closed)

  • Piotr Caban mentioned in merge request !4829 (merged)

    mentioned in merge request !4829 (merged)

  • The patch was merged in !4829 (merged). Thank you.

  • closed

  • Please register or sign in to reply
    Loading