Re: [Tinycc-devel] Use an anonymous file to back mmap

Hi Michael,

I noticed the break. After I tried to revert all my changes around mmap(), the test was still broken as the same as yours.

Looked into the issue further, I kept my change untouched and reverted two changes submitted between my changes as listed below. All tests were successful afterwards.

I am not very familiar with tcc code base yet, but it seems that it is more suspicious that the changes below broke the tests, instead of the mmap() one?

http://repo.or.cz/w/tinycc.git/commit/fdf9fba5785f5c0d285c8cbf2fa51df32ddd878d
http://repo.or.cz/w/tinycc.git/commit/ea7b17f641cb962d0e9a79137e93c7e1e24b99ce

Revert all mmap() changes

####################

Revert two changes listed above about fixing PE x86_64, while keeping mmap() change
#####################

------------ test3 ------------
../tcc -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -run tcctest.c > test.out3
Auto Test3 OK
gcc -o abitest-cc abitest.c ../libtcc.a -I.. -Wall -g -O2 -fno-strict-aliasing -Wno-pointer-sign -Wno-sign-compare -Wno-unused-result -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -lm -ldl -I..
../tcc -B.. -I.. -I.. -I../include -o abitest-tcc abitest.c ../libtcc.a -I.. -Wall -g -O2 -fno-strict-aliasing -Wno-pointer-sign -Wno-sign-compare -Wno-unused-result -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -lm -ldl -I..
------------ abitest ------------
./abitest-cc lib_path=.. include="../include"
ret_int_test... success
ret_longlong_test... success
ret_float_test... success
ret_double_test... success
ret_longdouble_test... success
ret_2float_test... success
ret_2double_test... success
reg_pack_test... success
reg_pack_longlong_test... success
sret_test... success
one_member_union_test... success
two_member_union_test... success
many_struct_test... success
many_struct_test_2... success
stdarg_test... success
stdarg_struct_test... success
arg_align_test... success
./abitest-tcc lib_path=.. include="../include"
ret_int_test... success
ret_longlong_test... success
ret_float_test... success
ret_double_test... success
ret_longdouble_test... success
ret_2float_test... success
ret_2double_test... success
reg_pack_test... success
reg_pack_longlong_test... success
sret_test... success
one_member_union_test... success
two_member_union_test... success
many_struct_test... success
many_struct_test_2... success
stdarg_test... success
stdarg_struct_test... success
arg_align_test... success
../tcc -B.. -I.. -I.. -I../include -o vla_test vla_test.c -I.. -Wall -g -O2 -fno-strict-aliasing -Wno-pointer-sign -Wno-sign-compare -Wno-unused-result
------------ vla_test-run ------------
./vla_test
test1... success
test2... success
test3... success
------------ moretests ------------
make -C tests2
make[2]: Entering directory '/home/keren/mob/tinycc/tests/tests2'
Test: 00_assignment...
Test: 01_comment...
Test: 02_printf...
Test: 03_struct...
Test: 04_for...
Test: 05_array...
Test: 06_case...
Test: 07_function...
Test: 08_while...
Test: 09_do_while...
Test: 10_pointer...
Test: 11_precedence...
Test: 12_hashdefine...
Test: 13_integer_literals...
Test: 14_if...
Test: 15_recursion...
Test: 16_nesting...
Test: 17_enum...
Test: 18_include...
Test: 19_pointer_arithmetic...
Test: 20_pointer_comparison...
Test: 21_char_array...
Test: 22_floating_point...
Test: 23_type_coercion...
Test: 24_math_library...
Test: 25_quicksort...
Test: 26_character_constants...
Test: 27_sizeof...
Test: 28_strings...
Test: 29_array_address...
Test: 31_args...
Test: 32_led...
Test: 33_ternary_op...
Test: 35_sizeof...
Test: 36_array_initialisers...
Test: 37_sprintf...
Test: 38_multiple_array_index...
Test: 39_typedef...
Test: 40_stdio...
Test: 41_hashif...
Test: 42_function_pointer...
Test: 43_void_param...
Test: 44_scoped_declarations...
Test: 45_empty_for...
Test: 47_switch_return...
Test: 48_nested_break...
Test: 49_bracket_evaluation...
Test: 50_logical_second_arg...
Test: 51_static...
Test: 52_unnamed_enum...
Test: 54_goto...
Test: 55_lshift_type...
make[2]: Leaving directory '/home/keren/mob/tinycc/tests/tests2'
make[1]: Leaving directory '/home/keren/mob/tinycc/tests'

And my reverted diff
##################
address@hidden tinycc]$ git diff
diff --git a/tccgen.c b/tccgen.c
index c7f0a87..f5ebcc5 100644
--- a/tccgen.c
+++ b/tccgen.c
@@ -1955,9 +1955,6 @@ static void gen_cast(CType *type)
                         s = 24;
                     else if ((dbt & VT_BTYPE) == VT_SHORT)
                         s = 16;
-#ifdef TCC_TARGET_X86_64
-                    if (!(dbt & (VT_PTR|VT_LLONG|VT_FUNC|VT_STRUCT)))
-#endif
                     if(dbt & VT_UNSIGNED)
                         vtop->c.ui = ((unsigned int)vtop->c.ll << s) >> s;
                     else
@@ -3908,11 +3905,7 @@ ST_FUNC void unary(void)
         /* if forward reference, we must point to s */
         if (vtop->r & VT_SYM) {
             vtop->sym = s;
-#ifdef TCC_TARGET_X86_64
-            vtop->c.ull = 0;
-#else
             vtop->c.ul = 0;
-#endif
         }
         break;
     }
@@ -5126,12 +5119,6 @@ static void init_putv(CType *type, Section *sec, unsigned long c,
         case VT_LLONG:
             *(long long *)ptr |= (vtop->c.ll & bit_mask) << bit_pos;
             break;
-        case VT_PTR:
-            if (vtop->r & VT_SYM) {
-                greloc(sec, vtop->sym, c, R_DATA_PTR);
-            }
-            *(addr_t *)ptr |= (vtop->c.ull & bit_mask) << bit_pos;
-            break;
         default:
             if (vtop->r & VT_SYM) {
                 greloc(sec, vtop->sym, c, R_DATA_PTR);

Best,
Keren

On Sat, Jan 11, 2014 at 10:41 AM, Michael Matz <address@hidden> wrote:

Hi,

On Thu, 9 Jan 2014, Keren Tan wrote:

I just submitted a tentative patch to the mob branch about mmap. When selinux is enabled, tccrun.c uses mmap to hold the dynamically generated code/data. It is backed by a randomly named file under /tmp directory. My patch is to use an anonymous file in mmap instead, so that the generated code/data only resides in memory, and tcc does not depend on a writable /tmp anymore.

It's customary to actually test changes before committing. In your case:

% ./configure --with-selinux
% make && make test
...
------------ test3 ------------
../tcc -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -run tcctest.c > test.out3
/bin/sh: line 1: 15954 Segmentation fault ../tcc -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -DCONFIG_LDDIR="\"lib64\"" -DTCC_TARGET_X86_64 -DONE_SOURCE -run ../tcc.c -B.. -I.. -I.. -I../include -run tcctest.c > test.out3
make[1]: *** [test3] Error 139

This is no wonder because the former mmap mechanism had some special requirements mandated by SElinux that you ignored. Your patch basically did this:

s1->write_mem = mmap (NULL, ret, PROT_READ|PROT_WRITE,
- MAP_SHARED, fd, 0);
+ MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);
...
s1->runtime_mem = mmap (NULL, ret, PROT_READ|PROT_EXEC,
- MAP_SHARED, fd, 0);
+ MAP_PRIVATE|MAP_ANONYMOUS, -1, 0);

So it replaced two mmaps of the same region (the file), one mapping being writable (pointer to it in ->write_mem), and another mapping being executable (pointer in ->runtime_mem) by two completely unrelated anon mappings, one writable, one executable.

The requirements of the whole things are such, that changes to the writable mapping via ->write_mem need to be reflected in the mapping reachable via ->runtime_mem. I.e. both mappings really need to be two views on _exactly_ the same data.

Depending on the configuration of an SElinux system the following things might be disallowed:
* creating mapping which is WRITE|EXEC
* changing flags of an existing writable mapping to include EXEC when it
didn't before (no matter if WRITE is now removed or not)

The first means you won't get away with just a single mmap of some ANON|PRIVATE that is writable and executable. The second means you also don't get away with first mapping something writable, let tcc generate its code therein and then remap it executable (either via mprotect or mremap). That is you indeed need two separate mappings. And because both mappings need to be actually from the same underlying data they need to be related somehow. And the only way to relate two separate mappings is via a file descriptor and a MAP_SHARED mapping.

IOW: the former way of mapping a writable and executable piece of memory via a shared mapping backed by a file is the only way that works in SElinux. And yes, that means it needs to have a place for that file, and it needs to be on a file system that isn't mounted noexec (i.e. /tmp might actually not be the right place).

In still other words: please revert, sorry. :-/

tcc is fantastic! I am new to this community. Your comments are very welcome!

Yeah, sorry to spoil the fun ;) Thanks for contributing, I hope the above at least explains things. Testing is the key :)

Ciao,
Michael.

From:	Keren Tan
Subject:	Re: [Tinycc-devel] Use an anonymous file to back mmap
Date:	Sat, 11 Jan 2014 14:36:16 -0800