[MSBuild] Preserve comments in build files & use LinkedList to store children #112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contribution by Paul Selormey
ProjectElementContainer should now handle modifications faster.
Unset attributes should return String.Empty, null is only for invalid.
grendello
added a commit
that referenced
this pull request
Jun 27, 2011
[MSBuild] Preserve comments in build files & use LinkedList to store children
ztzg
pushed a commit
to ztzg/mono
that referenced
this pull request
Jun 30, 2017
The code "callvirt[mono#112]" (used call to the virtual method mono#112) was splitted after the lowering pass into: iconst R25 <- [112] int_add R25 <- R22 R25 clobbers: 1 sh4_loadi4 R24 <- R25 call_reg ... As you can see, the opcode "int_add" uses the first register as destination ("clobbers: 1") so the register allocator had to move the virtual register R25 into R22 before the "int_add": mov mono#112,r0 ! mov r1,r0 add r0,r0 mov.l @r0,r0 jsr @r0 nop Finally, the function get_vcall_slot_addr() has to be adjusted.
lewurm
pushed a commit
to lewurm/mono
that referenced
this pull request
Jun 3, 2019
Fixes: mono#14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,mono#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Additional background information ================================= The original lucky, "good" behavior worked as follows for [mono/mono@725ba2a built with Android NDK r14][1]: 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,mono#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl mono#6 0x00000000000bc610 <+4120>: ldr x8, [x8,mono#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,mono#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,mono#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. The bad behavior for [mono/mono@725ba2a built with Android NDK r19][2] works just slightly differently: 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,mono#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,mono#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/
monojenkins
pushed a commit
to monojenkins/mono
that referenced
this pull request
Jun 3, 2019
Fixes: mono#14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,mono#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Additional background information ================================= The original lucky, "good" behavior worked as follows for [mono/mono@725ba2a built with Android NDK r14][1]: 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,mono#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl mono#6 0x00000000000bc610 <+4120>: ldr x8, [x8,mono#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,mono#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,mono#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. The bad behavior for [mono/mono@725ba2a built with Android NDK r19][2] works just slightly differently: 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,mono#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,mono#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/
monojenkins
pushed a commit
to monojenkins/mono
that referenced
this pull request
Jun 3, 2019
Fixes: mono#14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,mono#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Additional background information ================================= The original lucky, "good" behavior worked as follows for [mono/mono@725ba2a built with Android NDK r14][1]: 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,mono#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl mono#6 0x00000000000bc610 <+4120>: ldr x8, [x8,mono#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,mono#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,mono#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. The bad behavior for [mono/mono@725ba2a built with Android NDK r19][2] works just slightly differently: 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,mono#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,mono#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/
monojenkins
pushed a commit
to monojenkins/mono
that referenced
this pull request
Jun 3, 2019
Fixes: mono#14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,mono#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Additional background information ================================= The original lucky, "good" behavior worked as follows for [mono/mono@725ba2a built with Android NDK r14][1]: 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,mono#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl mono#6 0x00000000000bc610 <+4120>: ldr x8, [x8,mono#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,mono#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,mono#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. The bad behavior for [mono/mono@725ba2a built with Android NDK r19][2] works just slightly differently: 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,mono#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,mono#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/
monojenkins
pushed a commit
to monojenkins/mono
that referenced
this pull request
Jun 3, 2019
Fixes: mono#14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,mono#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Additional background information ================================= The original lucky, "good" behavior worked as follows for [mono/mono@725ba2a built with Android NDK r14][1]: 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,mono#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl mono#6 0x00000000000bc610 <+4120>: ldr x8, [x8,mono#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,mono#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,mono#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. The bad behavior for [mono/mono@725ba2a built with Android NDK r19][2] works just slightly differently: 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,mono#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,mono#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/
monojenkins
added a commit
that referenced
this pull request
Jun 3, 2019
[2018-08] [arm64] Correct exception filter stack frame size Fixes: #14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 <details> <summary>Additional background information</summary> ### The original lucky, "good" behavior for [725ba2a built with Android NDK r14][1] 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl #6 0x00000000000bc610 <+4120>: ldr x8, [x8,#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. ### The bad behavior for [725ba2a built with Android NDK r19][2] works slightly differently 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/ </details> Backport of #14757. /cc @lambdageek @brendanzagaeski
monojenkins
added a commit
that referenced
this pull request
Jun 3, 2019
[2019-02] [arm64] Correct exception filter stack frame size Fixes: #14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 <details> <summary>Additional background information</summary> ### The original lucky, "good" behavior for [725ba2a built with Android NDK r14][1] 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl #6 0x00000000000bc610 <+4120>: ldr x8, [x8,#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. ### The bad behavior for [725ba2a built with Android NDK r19][2] works slightly differently 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/ </details> Backport of #14757. /cc @lambdageek @brendanzagaeski
monojenkins
added a commit
that referenced
this pull request
Jun 4, 2019
[2019-06] [arm64] Correct exception filter stack frame size Fixes: #14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 <details> <summary>Additional background information</summary> ### The original lucky, "good" behavior for [725ba2a built with Android NDK r14][1] 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl #6 0x00000000000bc610 <+4120>: ldr x8, [x8,#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. ### The bad behavior for [725ba2a built with Android NDK r19][2] works slightly differently 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/ </details> Backport of #14757. /cc @lambdageek @brendanzagaeski
monojenkins
added a commit
that referenced
this pull request
Jun 4, 2019
[2018-10] [arm64] Correct exception filter stack frame size Fixes: #14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 <details> <summary>Additional background information</summary> ### The original lucky, "good" behavior for [725ba2a built with Android NDK r14][1] 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl #6 0x00000000000bc610 <+4120>: ldr x8, [x8,#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. ### The bad behavior for [725ba2a built with Android NDK r19][2] works slightly differently 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/ </details> Backport of #14757. /cc @lambdageek @brendanzagaeski
marek-safar
pushed a commit
that referenced
this pull request
Jun 5, 2019
* [arm64] Correct exception filter stack frame size Fixes: #14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Additional background information ================================= The original lucky, "good" behavior worked as follows for [725ba2a built with Android NDK r14][1]: 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl #6 0x00000000000bc610 <+4120>: ldr x8, [x8,#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. The bad behavior for [725ba2a built with Android NDK r19][2] works just slightly differently: 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/ * reduce frame size
alexanderkyte
pushed a commit
to alexanderkyte/mono
that referenced
this pull request
Jun 5, 2019
* [arm64] Correct exception filter stack frame size Fixes: mono#14170 Fixes: xamarin/xamarin-android#3112 The `call_filter()` function generated by `mono_arch_get_call_filter()` was overwriting a part of the previous stack frame because it was not creating a large enough new stack frame for itself. This had been working by luck before the switch to building the runtime with Android NDK r19. I used a local build of [xamarin/xamarin-android/d16-1@87a80b][0] to verify that this change resolved both of the issues linked above. I also confirmed that I was able to reintroduce the issues in my local environment by removing the change and rebuilding. After this change, the generated `call_filter()` function now starts by reserving sufficient space on the stack to hold a 64-bit value at the `ctx_offset` location. The 64-bit `ctx` value is saved to the `ctx_offset` location (offset 344 in the new stack frame) shortly afterwards: stp x29, x30, [sp,#-352]! mov x29, sp str x0, [x29,mono#344] As expected, the invocation of `call_filter()` now no longer modifies the top of the previous stack frame. Top of the stack before `call_filter()`: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Top of the stack after `call_filter()` (unchanged): (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 Additional background information ================================= The original lucky, "good" behavior worked as follows for [mono/mono@725ba2a built with Android NDK r14][1]: 1. The `resume` parameter for `mono_handle_exception_internal()` is held in register `w23`. 2. That register is saved into the current stack frame at offset 20 by a `str` instruction: 0x00000000000bc1bc <+3012>: str w23, [sp,mono#20] 3. `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction: 2279 filtered = call_filter (ctx, ei->data.filter); 0x00000000000bc60c <+4116>: add x9, x22, x24, lsl mono#6 0x00000000000bc610 <+4120>: ldr x8, [x8,mono#3120] 0x00000000000bc614 <+4124>: ldr x1, [x9,mono#112] 0x00000000000bc618 <+4128>: add x0, sp, #0x110 0x00000000000bc61c <+4132>: blr x8 Before the `blr` instruction, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x0a8b31d8 0x00000071 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 4. `call_filter()` runs. This function is generated by `mono_arch_get_call_filter()` and starts with: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] Note in particular how the first line subtracts 336 from `sp` to start a new stack frame, but the third line writes to a position that is 344 bytes beyond that, back within the *previous* stack frame. 5. After the invocations of `call_filter()` and `handle_exception_first_pass()`, `w23` is restored from the stack: 0x00000000000bc820 <+4648>: ldr w23, [sp,mono#20] At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x00000000 0x00000000 0x1ef51980 0x00000071 Notice how `call_filter()` has overwritten bytes 8 through 15 of the stack frame. In this original lucky scenario, this does not affect the value restored into register `w23` because that value starts at byte 20. 6. `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000bb960 <+872>: cbz w23, 0xbb9c4 <mono_handle_exception_internal+972> Since `w23` is `0`, `mono_handle_exception_internal()` correctly continues into the `else` branch. The bad behavior for [mono/mono@725ba2a built with Android NDK r19][2] works just slightly differently: 1. As before, the local `resume` parameter starts in register `w23`. 2. This time, the register is saved into the stack frame at offset 12 (instead of 20): 0x00000000000bed7c <+3200>: str w23, [sp,mono#12] 3. As before, `handle_exception_first_pass()` invokes `call_filter()` via a `blr` instruction. At this step, the top of the stack looks like this: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0x2724b700 0x00000000 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 4. `call_filter()` runs as before. And the first few instructions of that function are the same as before: stp x29, x30, [sp,#-336]! mov x29, sp str x0, [x29,mono#344] 5. `w23` is again restored from the stack, but this time from offset 12: 0x00000000000bf2c0 <+4548>: ldr w23, [sp,mono#12] The problem is that `call_filter()` has again overwritten bytes 8 through 15 of the stack frame: (gdb) x/8x $sp 0x7fcd2774a0: 0x7144d250 0x00000000 0xcd2775b0 0x0000007f 0x7fcd2774b0: 0x1ee19ff0 0x00000071 0x0cc00300 0x00000071 So after the `ldr` instruction, `w23` has an incorrect value of `0x7f` rather than the correct value `0`. 6. As before, `mono_handle_exception_internal()` tests the value of `w23` to decide how to set the `ji` local variable: 2574 if (resume) { 0x00000000000be430 <+820>: cbz w23, 0xbe834 <mono_handle_exception_internal+1848> But this time because `w23` is *not* zero, execution continues *incorrectly* into the `if` branch rather than the `else` branch. The result is that `ji` is set to `0` rather than a valid `(MonoJitInfo *)` value. This incorrect value for `ji` leads to a null pointer dereference, as seen in the bug reports. [0]: https://github.com/xamarin/xamarin-android/tree/xamarin-android-9.3.0.22 [1]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1432/Azure/ [2]: https://jenkins.xamarin.com/view/Xamarin.Android/job/xamarin-android-freestyle/1436/Azure/ * reduce frame size
picenka21
pushed a commit
to picenka21/runtime
that referenced
this pull request
Feb 18, 2022
[MSBuild] Preserve comments in build files & use LinkedList to store children Commit migrated from mono/mono@95a0730
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Preserving comments: Contribution by Paul Selormey from http://sandassist.codeplex.com/
LinkedList is what MSDN docs suggest for storing children of ProjectElementContainer and has better computational complexity for performing modifications.