Skip to content
Snippets Groups Projects

ntdll: Increment offset by len in build_clr_surrogate_section.

5 unresolved threads

Instead of multiplying it by 2. I don't know how this ever worked.

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54808

Merge request reports

Merge request pipeline #9896 skipped

Merge request pipeline skipped for 7af7ff87

Merged by Alexandre JulliardAlexandre Julliard 1 year ago (Apr 24, 2023 8:50pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Alex Henrie requested review from @nsivov

    requested review from @nsivov

  • Please add a kernel32 tests to trigger this. I think we only test a single entry, and that's why next offset does not matter for tests.

  • Actually, I don't see any test breakage with 7 records so far without this fix. Maybe we should test guid index fields as well, I think that was easy to expose since whole section pointer is returned.

  • Alex Henrie added 2 commits

    added 2 commits

    • 02195c4e - kernel32/tests: Check that lpSectionBase != NULL before dereferencing.
    • 35129b6b - ntdll: Increment offset by len in build_clr_surrogate_section.

    Compare with previous version

  • Author Developer

    Thanks for the guidance. I've pushed a new patchset that cleans up the existing header tests a bit and adds some new index tests that demonstrate the correctness of the change to build_clr_surrogate_section.

    • Contributor
      Resolved by Alex Henrie

      Marvin replied on the mailing list:

      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=131843
      
      Your paranoid android.
      
      
      === w7pro64 (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w864 (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w1064v1507 (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w1064v1809 (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w1064_2qxl (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w1064_adm (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w1064_tsign (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w10pro64 (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w10pro64_en_AE_u8 (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w10pro64_ar (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w10pro64_ja (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w10pro64_zh_CN (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
      
      === w11pro64_amd (64 bit report) ===
      
      kernel32:
      actctx.c:2823: Test failed: got error 14105
  • Nikolay Sivov
    Nikolay Sivov @nsivov started a thread on an outdated change in commit 35129b6b
1871 1878 ok_(__FILE__, line)(data.hActCtx == NULL, "data.hActCtx=%p\n", data.hActCtx);
1872 1879 ok_(__FILE__, line)(data.ulAssemblyRosterIndex == exid, "data.ulAssemblyRosterIndex=%lu, expected %lu\n",
1873 1880 data.ulAssemblyRosterIndex, exid);
1881
1882 header = (struct guidsection_header*)data.lpSectionBase;
1883 ok(header->magic == 0x64487347, "got wrong magic 0x%08lx\n", header->magic);
1884 ok(header->size == sizeof(*header), "got size %ld\n", header->size);
1885 ok(header->count == count, "got count %ld, expected %ld\n", header->count, count);
1886 ok(header->index_offset == sizeof(*header), "got index offset %lu\n", header->index_offset);
1887
1888 index = (struct guid_index*)((BYTE*)data.lpSectionBase + header->index_offset);
1889 ok(index[0].data_offset == sizeof(*header) + count * sizeof(*index), "got data offset %ld\n", index[0].data_offset);
1890 ok(index[0].rosterindex == exid, "got roster index %ld, expected %lu\n", index[0].rosterindex, exid);
1891 ok(index[1].data_offset == index[0].data_offset + index[0].data_len, "got data offset %lu, expected %lu\n",
1892 index[1].data_offset, index[0].data_offset + index[0].data_len);
1893 ok(index[1].rosterindex == exid, "got roster index %ld, expected %lu\n", index[1].rosterindex, exid);
  • Comment on lines +1881 to +1893

    I was thinking of having some helper that would validate guid index, and another one for string index (unrelated to your fix). There is no need to pass in number of entries, it's fine to use what's in the header. Then we'd call for index validation for all sections, it's possible there are more issues like what you've found.

  • Author Developer

    Today I tried writing generic tests to check any section header for consistency. The problem I ran into is that Windows is not consistent about where it puts blocks of data in the sections. Sometimes the blocks are not contiguous and sometimes they are out of order compared to the index. It would seem that the exact layout is unimportant.

    My preference would be to fix the obviously broken code now and worry about adding tests later. If the one-line fix cannot be accepted into mainline Wine without more tests, let's put it in Wine Staging until better tests are written.

  • It's not obvious, because tests are working for me without the fix somehow - if you add another clr entry, it is found correctly. I don't see why you'd mention wine-staging for this case if the fix is obvious to you, line count is not a metric to use.

    I'll take a look at additional tests that I suggested.

  • Please take a look. Testing that offset is within section bounds is enough to show what your fix changes. Note that your test is broken, because you can't have duplicate GUIDs in the same section. I'm not sure if we enforce that yet.

    0001-Some-tests.txt

  • Alex Henrie changed this line in version 3 of the diff

    changed this line in version 3 of the diff

  • Author Developer

    Thanks for the patch file. I've modified the tests that I started writing earlier to more closely resemble the ones you suggested. I hope that you are pleased with the result.

  • Please register or sign in to reply
  • Alex Henrie added 2 commits

    added 2 commits

    • afa39af2 - kernel32/tests: Test internal consistency of activation context headers.
    • 31106f6f - ntdll: Increment offset by len in build_clr_surrogate_section.

    Compare with previous version

    • Contributor

      Marvin replied on the mailing list:

      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=132041
      
      Your paranoid android.
      
      
      === w7pro64 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w864 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064v1507 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064v1809 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064_2qxl (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064_adm (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064_tsign (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64_en_AE_u8 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64_ar (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64_ja (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64_zh_CN (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w11pro64_amd (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
  • Alex Henrie added 258 commits

    added 258 commits

    • 31106f6f...cd9f3e5d - 256 commits from branch wine:master
    • c400db7d - kernel32/tests: Test internal consistency of activation context headers.
    • 0e129f76 - ntdll: Increment offset by len in build_clr_surrogate_section.

    Compare with previous version

    • Contributor

      Marvin replied on the mailing list:

      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=132042
      
      Your paranoid android.
      
      
      === w7pro64 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w864 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064v1507 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064v1809 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064_2qxl (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064_adm (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w1064_tsign (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64_en_AE_u8 (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64_ar (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64_ja (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w10pro64_zh_CN (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
      
      === w11pro64_amd (64 bit report) ===
      
      kernel32:
      actctx.c:2874: Test failed: got error 14105
  • Alex Henrie added 2 commits

    added 2 commits

    • 9a6a34b9 - kernel32/tests: Check that lpSectionBase != NULL before dereferencing.
    • ef28221f - ntdll: Increment offset by len in build_clr_surrogate_section.

    Compare with previous version

    • Contributor

      Marvin replied on the mailing list:

      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=132073
      
      Your paranoid android.
      
      
      === w7pro64 (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w864 (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w1064v1507 (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w1064v1809 (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w1064_2qxl (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w1064_adm (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w1064_tsign (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w10pro64 (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w10pro64_en_AE_u8 (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w10pro64_ar (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w10pro64_ja (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w10pro64_zh_CN (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
      
      === w11pro64_amd (64 bit report) ===
      
      kernel32:
      actctx.c:2839: Test failed: got error 14105
  • Nikolay Sivov approved this merge request

    approved this merge request

  • Alexandre Julliard added 38 commits

    added 38 commits

    • ef28221f...2371906a - 36 commits from branch wine:master
    • bd745957 - kernel32/tests: Check that lpSectionBase != NULL before dereferencing.
    • 7af7ff87 - ntdll: Increment offset by len in build_clr_surrogate_section.

    Compare with previous version

  • Alexandre Julliard approved this merge request

    approved this merge request

  • Please register or sign in to reply
    Loading