{"action":"created","issue":{"url":"https://api.github.com/repos/alainfrisch/flexdll/issues/57","repository_url":"https://api.github.com/repos/alainfrisch/flexdll","labels_url":"https://api.github.com/repos/alainfrisch/flexdll/issues/57/labels{/name}","comments_url":"https://api.github.com/repos/alainfrisch/flexdll/issues/57/comments","events_url":"https://api.github.com/repos/alainfrisch/flexdll/issues/57/events","html_url":"https://github.com/alainfrisch/flexdll/pull/57","id":344462608,"node_id":"MDExOlB1bGxSZXF1ZXN0MjAzODUyMTI4","number":57,"title":"Fix page access rights bug","user":{"login":"db4","id":6112699,"node_id":"MDQ6VXNlcjYxMTI2OTk=","avatar_url":"https://avatars3.githubusercontent.com/u/6112699?v=4","gravatar_id":"","url":"https://api.github.com/users/db4","html_url":"https://github.com/db4","followers_url":"https://api.github.com/users/db4/followers","following_url":"https://api.github.com/users/db4/following{/other_user}","gists_url":"https://api.github.com/users/db4/gists{/gist_id}","starred_url":"https://api.github.com/users/db4/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/db4/subscriptions","organizations_url":"https://api.github.com/users/db4/orgs","repos_url":"https://api.github.com/users/db4/repos","events_url":"https://api.github.com/users/db4/events{/privacy}","received_events_url":"https://api.github.com/users/db4/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2018-07-25T14:15:25Z","updated_at":"2018-08-01T15:43:17Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/alainfrisch/flexdll/pulls/57","html_url":"https://github.com/alainfrisch/flexdll/pull/57","diff_url":"https://github.com/alainfrisch/flexdll/pull/57.diff","patch_url":"https://github.com/alainfrisch/flexdll/pull/57.patch"},"body":"My original report in email to Alain Frisch, Nov 25 2013:\r\n```\r\nHi Alain,\r\n\r\nI stumbled across an access violation bug caused by the way FlexDLL\r\nperforms relocations. When constructing dynamic relocation tables it\r\npacks several consecutive relocation addresses into one \"zone\". When\r\nperforming actual relocations it allows write access via\r\nVirtualProtect(zone_begin, zone_size, PAGE_EXECUTE_WRITECOPY) (BTW,\r\nwhy not PAGE_EXECUTE_READWRITE?), then patches addresses and then\r\nrestore the old access rights via VirtualProtect(zone_begin,\r\nzone_size, old_access). Unfortunately this is not entirely correct. I\r\nsee the situation where one zone covers 2 memory pages with different\r\naccess rights: PAGE_READONLY and PAGE_EXECUTE_READ. On restore the\r\nsecond page will be set PAGE_READONLY and any attempt to execute code\r\nfrom it will lead to an access violation error (as CPU supports NX and\r\nDEP is enabled in the OS).\r\n\r\nRelocation table that causes troubles:\r\n\r\nDynamic relocation table found at 634B13A0\r\n Non-writable relocation in zone 63483D57 -> 63483E83\r\n Non-writable relocation in zone 63483BB8 -> 63483CD5\r\n Non-writable relocation in zone 63480BE5 -> 634811B8\r\n 634811B8 (kind:0100) (now:00C363E9)  caml_local_roots\r\n 634811B4 (kind:0100) (now:00005E58)  caml_local_roots\r\n 63480BE9 (kind:0100) (now:00000000)  caml_atom_table\r\n 63480BE5 (kind:0100) (now:00000000)  caml_atom_table\r\n 63483CD5 (kind:0102) (now:013D2900)  caml_copy_string\r\n 63483CB8 (kind:0101) (now:0032D3F8)  w32_unicode_pack_to_string\r\n 63483CA5 (kind:0102) (now:013FC224)  caml_atom_table\r\n 63483C8C (kind:0101) (now:9DF62F20)  caml_initialize\r\n 63483C4E (kind:0101) (now:9DF62BFE)  caml_alloc_shr\r\n 63483BE9 (kind:0101) (now:0032D526)  w32_uuid_ml2c\r\n 63483BB8 (kind:0101) (now:0032D4CB)  w32_uuid_c2ml\r\n 63483E83 (kind:0102) (now:013FB4D0)  caml_local_roots\r\n 63483E68 (kind:0102) (now:013FB4D0)  caml_local_roots\r\n 63483D81 (kind:0102) (now:013FB4D0)  caml_local_roots\r\n 63483D75 (kind:0102) (now:013FB4D0)  caml_local_roots\r\n 63483D57 (kind:0102) (now:013FB4D0)  caml_local_roots\r\n\r\n63480000 page was PAGE_READONLY\r\n63481000 page was PAGE_EXECUTE_READ\r\n\r\nAnother problem is that one page can be covered by several zones. As\r\nyou restore access rights in the same order you modify them, they will\r\nbe incorrect in the end. I was able to fix this just replacing\r\n\r\n  /* Restore permissions. Should do it also in case of failure... */\r\n  for (wr = tbl->nonwr; wr->last != 0; wr++)\r\n    allow_write(wr->first,wr->last + 4,wr->old,&wr->old);\r\n\r\nwith\r\n\r\n  /* Restore permissions in the reverse order. Should do it also in\r\ncase of failure... */\r\n  wr = tbl->nonwr;\r\n  while ((wr+1)->last != 0)\r\n    wr++;\r\n  for (; wr >= tbl->nonwr; wr--)\r\n    allow_write(wr->first,wr->last + 4,wr->old,&wr->old);\r\n\r\nbut I still have no idea how to fix the first problem. Now I just use\r\n\r\n    allow_write(wr->first,wr->last + 4,(wr->old ==\r\nPAGE_READONLY)?PAGE_EXECUTE_READ:wr->old,&wr->old);\r\n\r\nas a workaround in hope that someone will fix it properly.\r\n```\r\nAlain's response:\r\n```\r\n1. First version:  one changes the right accesses for each relocation and immediately restore it after the patch is done.  This even simplifies the work of flexlink.exe, since it no longer needs to compute these \"zones\".\r\n\r\n2. Second version: *if* it turns out to be too expensive to change the rights on memory pages for each relocation, one can implement the simplest strategy of checking if the next relocation falls in the same page as the previous one, and in this case, avoid the two useless calls to VirtualProtect.  One should then arrange so that flexlink.exe emits relocations in a sorted order (it might already be the case). \r\n```\r\n> I think one should evaluate the performance impact of changing access rights on each relocation. There can be many relocations, and I'm concerned that this could introduce a significant slowdown when loading a DLL. Have you tried to assess the performance impact?\r\n\r\nNow I can prove that the performance impact of the second (slow) variant is negligible.  I see the following figures for test project then FLEXDLL_DEBUG is enabled:\r\n\r\n> allow_write() calls: 4, total time: 0.000029 sec\r\n\r\nSo one `VIrtualProtect()` call takes about 7 microseconds on my hardware. For a real DLL with even hundreds of relocations it would be several milliseconds, not more."},"comment":{"url":"https://api.github.com/repos/alainfrisch/flexdll/issues/comments/409620840","html_url":"https://github.com/alainfrisch/flexdll/pull/57#issuecomment-409620840","issue_url":"https://api.github.com/repos/alainfrisch/flexdll/issues/57","id":409620840,"node_id":"MDEyOklzc3VlQ29tbWVudDQwOTYyMDg0MA==","user":{"login":"alainfrisch","id":3305274,"node_id":"MDQ6VXNlcjMzMDUyNzQ=","avatar_url":"https://avatars0.githubusercontent.com/u/3305274?v=4","gravatar_id":"","url":"https://api.github.com/users/alainfrisch","html_url":"https://github.com/alainfrisch","followers_url":"https://api.github.com/users/alainfrisch/followers","following_url":"https://api.github.com/users/alainfrisch/following{/other_user}","gists_url":"https://api.github.com/users/alainfrisch/gists{/gist_id}","starred_url":"https://api.github.com/users/alainfrisch/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/alainfrisch/subscriptions","organizations_url":"https://api.github.com/users/alainfrisch/orgs","repos_url":"https://api.github.com/users/alainfrisch/repos","events_url":"https://api.github.com/users/alainfrisch/events{/privacy}","received_events_url":"https://api.github.com/users/alainfrisch/received_events","type":"User","site_admin":false},"created_at":"2018-08-01T15:43:16Z","updated_at":"2018-08-01T15:43:16Z","author_association":"OWNER","body":"See #64.  @db4 does this look good to you?"}}
{"actor":{"display_login":"alainfrisch"}}