Ticket #473 (closed bug: fixed)

Opened 6 years ago

Last modified 5 years ago

Data Execution Prevention breaks "perl Configure.pl"; remove Parrot_memcpy_aligned

Reported by: davew Owned by: jkeenan
Priority: normal Milestone:
Component: configure Version: trunk
Severity: medium Keywords: DEP Configure
Cc: particle, fperrad, mikehh Language:
Patch status: applied Platform:

Description

I have Data Execution Prevention enabled on Windows XP. When I run perl Configure.pl, it fails in a couple places with DEP exceptions:

auto::jit - Determine JIT capability...
auto::cpu - Generate CPU specific stuff... (2 times)

Looking at one of the .c files that is compiled and run during the Configure.pl process, I can see the data execution.

static const char Parrot_memcpy_aligned_sse_code[] = {
                           /* Parrot_memcpy_aligned_sse: */
    0x57,                       /* pushl %edi */
    0x56,                       /* pushl %esi */
    0x8B, 0x44, 0x24, 0x0C,     /* movl 12(%esp), %eax */
    0x8B, 0x4C, 0x24, 0x14,     /* mov 20(%esp), %ecx */
    0x8B, 0x74, 0x24, 0x10,     /* mov 16(%esp), %esi */
    0x89, 0xC7,                 /* mov %eax, %edi */
    0xC1, 0xE9, 0x05,           /* shr $5, %ecx */
                           /* 1: */
    0x0F, 0x10, 0x06,           /* movups 0(%esi), %xmm0 */
    0x0F, 0x10, 0x4E, 0x10,     /* movups 16(%esi), %xmm1 */
    0x83, 0xC6, 0x20,           /* add $32, %esi */
    0x0F, 0x11, 0x07,           /* movups %xmm0, 0(%edi) */
    0x0F, 0x11, 0x4F, 0x10,     /* movups %xmm1, 16(%edi) */
    0x83, 0xC7, 0x20,           /* add $32, %edi */
    0x49,                       /* dec %ecx */
    0x75, 0xE9,                 /* jnz 1b */
    0x5E,                       /* popl %esi */
    0x5F,                       /* popl %edi */
    0xC3,                       /* ret */
    0x00
};

typedef void* (*Parrot_memcpy_aligned_sse_t)(void *dest, const void *src, size_t);

  .
  .
  .

Parrot_memcpy_aligned_sse_t Parrot_memcpy_aligned_sse =
    (Parrot_memcpy_aligned_sse_t) Parrot_memcpy_aligned_sse_code;

  .
  .
  .

Parrot_memcpy_aligned_sse(d, s, n);

A data segment-based array is cast to a function pointer that is subsequently called, hence the DEP exception.

I recommend that on windoze platforms only, the data at Parrot_memcpy_aligned_sse_code be copied to a buffer allocated with VirtualAlloc(), then set execution privs on that buffer with VirtualProtect(PAGE_EXECUTE), and finally call the machine code at the VirtualAlloc address. It's a lot of rig-a-ma-roll, but it lets you execute arbitrary data without fear of DEP. Here is a link that discusses the issue:

 http://msdn.microsoft.com/en-us/library/aa366553(VS.85).aspx

After disabling DEP and rebooting, I was able to successfully configure, compile, and run Parrot 1.0! The DEP errors are scary and vague looking enough that this bug might need to be addressed.

Cheers,
Dave Woldrich
dave@…

Attachments

remove_memcpy_aligned.patch Download (15.5 KB) - added by doughera 6 years ago.
t.library.test_more.t.failure.txt Download (5.4 KB) - added by jkeenan 5 years ago.
Failure when trying to merge tt473_remove_memcpy_aligned branch into trunk

Change History

  Changed 6 years ago by jkeenan

  • cc particle, fperrad added

I guess this shows the virtue of publicizing a release as '1.0': You get people trying to build Parrot in ways we never imagined in several years of development!

Could some of our developers who spend a lot of time on Win32 take a look at this?

Thank you very much for your report.
kid51

  Changed 6 years ago by doughera

The two auto::cpu entries are easy: Configure.pl goes to a lot of effort probing for things related to Parrot_memcpy_aligned. However Parrot_memcpy_aligned is never used. The tests may simply be deleted.

The jit test is harder. There's a comment:

      RT #43146 use executable memory for this test if needed

but no explanation of why or why not it might be "needed". Perhaps it's obvious to someone else. (And no, that ticket doesn't contain any further information at all.)

If the goal is simply to determine if the 'fcomip' instruction is available, then there are surely better ways to feed assembly language instructions to the compiler. If the goal is to actually test whether data can be executed, then that's a different story. Parrot shouldn't trigger alarming pop-up warnings. We'll need to make a Windows-specific probe to call whatever function there might be to learn about DEP. Alas there, I have no idea.

  Changed 6 years ago by doughera

I have attached a patch to remove the two problematic entries in auto::cpu::i386. Depending on how your version of 'patch' works, you may have to manually delete the empty files in config/auto/cpu/i386/.

There were three main problems with these probes:

First, and foremost, they were useless. The results were never used.

Second, if Configure.pl wishes to probe for specific assembly instructions, there ought to be friendlier ways to pass assembly instructions to the compiler. Most compilers accept some form of "asm()" instruction.

Third, if they were actually used, these probes would make it more difficult for packagers to build a parrot on one machine and have it run on a variety of similar, but not identical, machines. For example, if the build machine has the SSE instruction, but the machine running the eventual package does not, then parrot would simply fail. If there were a Configure.pl command-line switch to override the probed values, then the packager could set it, but there is no such switch.

Changed 6 years ago by doughera

follow-up: ↓ 5   Changed 5 years ago by jkeenan

  • owner set to jkeenan
  • status changed from new to assigned

Reviewing tickets that have not been touched in many months, I came across this patch and created the tt473_remove_memcpy_aligned branch in the repository to evaluate it. It has passed make test (see  this Smolder report for Linux/i386. Am currently running make fulltest.

Can we get smoke tests of this branch on other i386 systems: Win32, Darwin and FreeBSD in particular?

Thank you very much.
kid51

in reply to: ↑ 4   Changed 5 years ago by jkeenan

Replying to jkeenan:

Am currently running make fulltest.

I corrected SVN status problems that were preventing make manifest_tests from passing. Apart from that, the only other failure in make fulltest was one in the examples test that was present in trunk when the branch was created (see TT #1384).

kid51

  Changed 5 years ago by dukeleto

  Changed 5 years ago by jkeenan

I was all set to merge the tt473_remove_memcpy_aligned branch into trunk. JimmyZ had gotten a PASS on make test on Win32. I decided to do one last make test on Linux -- only to get a big failure in, of all places, t/library/test_more.t. See attachment. Haven't diagnosed it yet.

kid51

Changed 5 years ago by jkeenan

Failure when trying to merge tt473_remove_memcpy_aligned branch into trunk

  Changed 5 years ago by jkeenan

  • cc mikehh added
  • platform win32 deleted

Today I again tried to merge this branch into trunk. The failure in t/library/test_more.t somehow cleared itself up -- but I got a different failure in t/compilers/pct/complete_workflow.t.

[li11-226:formerge] 543 $ perl t/harness --gc-debug t/compilers/pct/complete_workflow.t
t/compilers/pct/complete_workflow.t .. 48/54 src/gc/api.c:245: failed assertion 'PObj_is_PMC_TEST(obj)'
Backtrace - Obtained 31 stack frames (max trace depth is 32).
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400e4572]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_confess+0x9a) [0x400e46da]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_gc_mark_PMC_alive_fun+0x89) [0x400f2b89]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x40287dcf]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_gc_mark_PMC_alive_fun+0x116) [0x400f2c16]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x40287fe7]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_gc_mark_PMC_alive_fun+0x116) [0x400f2c16]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400f6dd9]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400f50e2]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400f53a5]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400f28a8]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400f4f7d]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400f4df4]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400f292b]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x400f32a8]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_str_new_COW+0x161) [0x40053a81]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_str_copy+0x78) [0x40053bb8]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x402730dd]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x4027329d]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x4006894e]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x4015fc2e]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x4015e18f]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x401077bf]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_pcc_invoke_from_sig_object+0x1e9) [0x400fdcc9]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_pcc_invoke_sub_from_c_args+0xd3) [0x400fddd3]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(Parrot_runcode+0x15e) [0x400e11de]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0 [0x402f5c69]
/home/jimk/work/formerge/blib/lib/libparrot.so.1.9.0(imcc_run+0x39c) [0x402f685c]
/home/jimk/work/formerge/parrot [0x8048988]
/lib/libc.so.6(__libc_start_main+0xe5) [0x40f57455]
/home/jimk/work/formerge/parrot [0x8048821]

#   Failed test 'our and key: generated PIR successfully'
#   at t/compilers/pct/complete_workflow.t line 408.
#          got: '[SIGNAL 6]'
#     expected: '0'

#   Failed test 'our and key: generated actions file exists'
#   at t/compilers/pct/complete_workflow.t line 409.
open '/home/jimk/work/formerge/t/compilers/pct/GJRUIqlWy7.pir': No such file or directory at lib/Parrot/BuildUtil.pm line 88.
# Looks like you planned 54 tests but ran 53.
# Looks like you failed 2 tests of 53 run.
# Looks like your test exited with 2 just after 53.
t/compilers/pct/complete_workflow.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 3/54 subtests 

Test Summary Report
-------------------
t/compilers/pct/complete_workflow.t (Wstat: 512 Tests: 53 Failed: 2)
  Failed tests:  52-53
  Non-zero exit status: 2
  Parse errors: Bad plan.  You planned 54 tests but ran 53.
Files=1, Tests=53,  6 wallclock secs ( 0.02 usr  0.00 sys +  3.27 cusr  0.22 csys =  3.51 CPU)
Result: FAIL

This error is shockingly similar to the one I have reported in TT #1393, wherein, since r43211, t/compilers/pge/pge_examples.t fails when run through the harness with --gc-debug. Both errors start out with:

# src/gc/api.c:245: failed assertion 'PObj_is_PMC_TEST(obj)'

The first 16 frames in their backtraces show Parrot calls in the same sequences:

# /libparrot.so.1.9.0 [0x400e45b2]
# /libparrot.so.1.9.0 (Parrot_confess+0x9a) [0x400e471a]
# /libparrot.so.1.9.0 (Parrot_gc_mark_PMC_alive_fun+0x89) [0x400f2c99]
# /libparrot.so.1.9.0 [0x40287f7f]
# /libparrot.so.1.9.0  (Parrot_gc_mark_PMC_alive_fun+0x116) [0x400f2d26]
# /libparrot.so.1.9.0 [0x40288197]
# /libparrot.so.1.9.0 (Parrot_gc_mark_PMC_alive_fun+0x116) [0x400f2d26]
# /libparrot.so.1.9.0 [0x400f6ee9]
# /libparrot.so.1.9.0 [0x400f51f2]
# /libparrot.so.1.9.0 [0x400f54b5]
# /libparrot.so.1.9.0 [0x400f29b8]
# /libparrot.so.1.9.0 [0x400f508d]
# /libparrot.so.1.9.0 [0x400f4f04]
# /libparrot.so.1.9.0 [0x400f2a3b]
# /libparrot.so.1.9.0 [0x400f33b8]
# /libparrot.so.1.9.0 (Parrot_str_new_COW+0x8f) [0x400539ef]

... and there are plenty of similarities in the remaining 16 frames. The branch I am attempting to merge into trunk was forked from trunk at r43163, i.e., before r43211. The branch passes all its own tests, so the errors reported above are almost certainly due to recent changes in trunk, with r43211 again a likely culprit.

Thank you very much.
kid51

follow-up: ↓ 12   Changed 5 years ago by jkeenan

  • summary changed from Data Execution Prevention breaks "perl Configure.pl" to Data Execution Prevention breaks "perl Configure.pl"; remove Parrot_memcpy_aligned
  • patch set to applied

I took advantage of the fact that the test failures reported in TT #1393 have disappeared, albeit perhaps only momentarily, to try merging this branch into trunk again. This time, I have been successful with make test. So I merged the branch into trunk and committed in r43441.

I will keep the ticket open for several days to receive reports of problems.

Thank you very much.

kid51

follow-up: ↓ 11   Changed 5 years ago by jkeenan

I see that we're getting one codingstd test failure subsequent to this merge:

$ prove -v t/codingstd/c_header_guards.t 
t/codingstd/c_header_guards.t .. 
1..5
not ok 1 - identical PARROT_*_GUARD macro names used in headers

#   Failed test 'identical PARROT_*_GUARD macro names used in headers'
#   at t/codingstd/c_header_guards.t line 131.
# collisions: 
# /home/jimk/work/parrot/include/parrot/platform_interface.h, 
# /home/jimk/work/parrot/config/gen/platform/platform_interface.h
ok 2 - multiple PARROT_*_GUARD macros found in headers
ok 3 - missing or misspelled PARROT_*_GUARD ifndef in headers
ok 4 - missing or misspelled PARROT_*_GUARD define in headers
ok 5 - missing or misspelled PARROT_*_GUARD comment after the endif in headers
# Looks like you failed 1 test of 5.
Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/5 subtests 

Test Summary Report
-------------------
t/codingstd/c_header_guards.t (Wstat: 256 Tests: 5 Failed: 1)
  Failed test:  1
  Non-zero exit status: 1
Files=1, Tests=5,  1 wallclock secs 
  ( 0.02 usr  0.00 sys +  0.22 cusr  0.00 csys =  0.24 CPU)
Result: FAIL

I'm off to $job now, so I would appreciate anyone who can take a look at this.

Thank you very much.

kid51

in reply to: ↑ 10   Changed 5 years ago by jkeenan

Replying to jkeenan:

I now think that this was an artifact of testing. A make realclean and reconfigure and rebuild cleared the problem up. So this removes one barrier to resolving this TT.

Thank you very much.

kid51

in reply to: ↑ 9   Changed 5 years ago by jkeenan

  • status changed from assigned to closed
  • resolution set to fixed

Replying to jkeenan:

I will keep the ticket open for several days to receive reports of problems.

The only problems reported were those I reported myself. And those were largely spurious. Closing ticket.

Thank you very much.

kid51

Note: See TracTickets for help on using tickets.