Eliminates run-time initialization of global variables (fixes race condition) #1171

tmfink · 2018-06-09T04:15:53Z

Declare global arch arrays with contents.

This eliminates the need for archs_enable() and eliminates the racey
initialization.

Progress:

declare global arrays with values

danghvu · 2018-06-09T16:01:55Z

I think this PR is overkill, adding a dependency on -pthread to fix, why not a simple spinlock/mutex ? The flag CAPSTONE_THREAD_SAFE is also unnecessary, capstone should be thread-safe without any flag (given that each thread uses its own handle that is); it's a bug that it's not, we should fix it.

aquynh · 2018-06-09T16:45:34Z

Yes i think we can do better than introducing -pthread dependency. And we should be threadsafe by default, not by choice.

tmfink · 2018-06-09T20:43:38Z

I agree that the libpthread dependency is undesirable, but I'm not sure of an easy-to-maintain solution that will work on Unix-based platforms.

Note: I am not a threading expert

Safely doing a concurrent initialization is not actually simple. We could try to maintain our own "pthread_once()" implementation, but doing so would require us to also pull in a spin lock and memory barrier implementation from somewhere like GNU libpthread.

Using a mutex would also have the downsides described above.

GNU libpthread pthread_once() implementation:
https://git.savannah.gnu.org/cgit/hurd/libpthread.git/tree/sysdeps/generic/pt-once.c

Edit: GNU libpthread is LGPL, not GPL. There is no problem dynamic linking against LGPL code.

tmfink · 2018-06-09T20:47:06Z

In regards to being enabling CAPSTONE_THREAD_SAFE by default:

If we end up dynamic linking against another library (such as pthread), then I do not think we should enable CAPSTONE_THREAD_SAFE by default.
Linking against another library is a breaking change since users' existing builds could fail if they are not already linking against libpthread..

tmfink · 2018-06-09T20:48:10Z

@danghvu @aquynh Any thoughts on where we can find a BSD compatible threading library that at least works on Unix-like platforms?

tmfink · 2018-06-09T21:03:46Z

We could borrow code from FreeBSD's threading library:

https://github.com/freebsd/freebsd/blob/260ba0bff18bb32b01216d6870c8273cf22246a7/lib/libthr/thread/thr_once.c#L66

danghvu · 2018-06-09T21:54:54Z

If we end up dynamic linking against another library (such as pthread), then I do not think we should enable CAPSTONE_THREAD_SAFE by default.
Linking against another library is a breaking change since users' existing builds could fail if they are not already linking against libpthread..

I got your point, but again, I think the philosophy is for capstone to work by default with thread. If including external library is what it takes, we may just do it; I don't think it is the case though.

My take is that for generic stuff like pthread_once, it has to worry of cases like long running function, re-entrance, thread yielding, etc.. which can be complicated.
In our case (you can prove me wrong) this code is short and simple, so a spinlock which mostly requires (portable) test-and-set and a memory barrier is enough. Maybe the hard part is to find one with the right LICENSE.

Btw, this code is initially written using __attribute__((constructor)), but changed to this to make it more portable; it's also an option.

tmfink · 2018-06-11T01:29:55Z

After thinking about this problem some more, I believe that we should dynamically link against libpthread:

Note: I incorrectly stated above that GNU libpthread is GPL; it is actually LGPL licensed.

On just about any Unix-like platform where we would want to use libpthread, libpthread should be available. What platform provides libc but not libpthread?
- I'm therefore also okay with enabling CAPSTONE_THREAD_SAFE by default. Doing so is technically a breaking change, but in the interest of safety, it is the best option.
- We just need to update documentation and examples of that link against Capstone to also link against pthread.
Just about any libpthread implementation should allow dynamic linking.
- GNU Hurd pthread?: LGPLv2 or later
- Glibc pthread: LGPLv2.1 or later
- FreeBSD libthr: BSD 2-clause

tmfink · 2018-06-13T17:03:13Z

Instead of adding threading support, the latest update just declares the global variables with the correct values. No run-time initialization is required.

danghvu · 2018-06-13T19:46:33Z

Thanks! This new update is much nicer! I fully support it, @aquynh should take a look. Could you please add a note on how to regenerate the generated files using the python script ? Or maybe an option in makefile to re-generate them when needed.

radare · 2018-06-13T21:07:05Z

thank you for bringing back the sanity

…

On 13 Jun 2018, at 19:03, Travis Finkenauer ***@***.***> wrote: Instead of adding threading support, the latest update just declares the global variables with the correct values. No run-time initialization is required. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA3-lq6Fs8OaTdax1t2N7tQXbaYjcRbJks5t8UXTgaJpZM4UhKu9>.

aquynh · 2018-06-13T22:12:15Z

generate_cs_arch_globals.py

+
+HEADER = """
+/* Capstone Disassembly Engine */
+/* By Nguyen Anh Quynh <aquynh@gmail.com>, {year} */


no, this is for auto-generated file, not created by me ;-)

to credit, please add your name into this new file (generate_cs_arch_globals.py) if you want that.

Thanks, done.

tmfink · 2018-06-14T06:42:20Z

I don't trust myself to update the mscv orxcode build directories.

stevemk14ebr · 2018-06-15T01:24:41Z

you should be using atomic bool compiler intrisics to just set the flag (or std::atomic if you want to go to c++ land 😜 ). __sync_lock_test_and_set on a gloabl instead of a local static bool and you are good to go with minimal changes. Would need to #ifdef some stuff for different compilers but still better than all these changes

danghvu · 2018-06-15T01:53:56Z

@tmfink can we make this cs_arch_globals.c into a header file ? cs_arch_globals.h and include in cs.c, then you don't need to care about how the other xcode or mscv has to be updated to compile your file.

@stevemk14ebr well, __sync_lock_test_and_set is only GNU extension; it may be a simple change but need to be careful about portability. I would prefer this compile time initialization, than having to deal with atomic issues.

stevemk14ebr · 2018-06-15T02:53:53Z

every compiler has an intrinisic for a fetch, just named different. Why in the world would you prefer a whole script to run at compiler time when you can just use 4 or 5 ifdefs and a global. Use the simple tool for the simple problem

danghvu · 2018-06-15T03:09:29Z

Why in the world would you prefer a whole script to run at compiler time when you can just use 4 or 5 ifdefs and a global.

It's not "a whole script to run at compiler time", the file is generated once not while compiling. It is also saving runtime cycles, and simple enough to understand, bullet-proof regarding thread-safe.

Use the simple tool for the simple problem

Why don't you submit a PR so we can judge ? It could be only my personal opinion without seeing the actual alternative.

stevemk14ebr · 2018-06-15T04:01:26Z

I'm here to suggest alternative and simpler designs, out of band source generation should be reserved for features that cannot be implemented in pure C, which it can be imho. I'm not writing any code for this project until previous time I invested is merged: #520 (just to explain why since you asked)

danghvu · 2018-06-15T04:16:40Z

@stevemk14ebr :) thanks for the contribution, though I don't understand why you bring that PR here, it has nothing to do with this, and I also didn't ask for it. To avoid dragging this off-topic, I'm done with my argument, should hear from others.
EDIT: Maybe it's my impoliteness asking for your code so you bring that up here, pardon me then; I'm just very doubtful that it is easy as you said.

tmfink · 2018-06-15T08:04:27Z

@tmfink can we make this cs_arch_globals.c into a header file ? cs_arch_globals.h and include in cs.c, then you don't need to care about how the other xcode or mscv has to be updated to compile your file.

@danghvu That would work but would be kinda ugly. Semantically, the cs_arch_globals file would be a .c file. To include it in another .c file would not be pretty.

We can let @aquynh decide how to proceed here.

tmfink · 2018-06-15T08:21:11Z

@stevemk14ebr I think that doing the compile-time initialization is preferable because:

Eliminates any dependencies/complication with threading
- Using intrinsics is not portable between compilers
More efficient (no need call to an initialization function that potentially blocks other threads)
The Python script only needs run if the Python script is modified
- The Unix Makefile will only possibly run the script; if the CHECK_CS_ARCH_GLOBALS variable is set

An alternative (that I considered) was to not include the Python script. The cs_arch_globals.c file would just need to be maintained manually. I decided not to do this because I felt that modifying the .c file is more error-prone than modifying the script.

aquynh · 2018-06-15T22:34:50Z

i am travelling, sorry it will take a bit longer to review this.

but personally i prefer compiler independent solution, so we dont need to update the code to support new compilers in the future.

aquynh · 2018-06-20T14:18:21Z

great work, @tmfink!

the remaining work is to add cs_arch_globals.c msvc/ & xcode/, so they dont break. can you do that, or need helps?

tmfink · 2018-06-20T16:28:26Z

Thanks @aquynh. I think I need help with the xcode/msvc projects.

Maintainers should be able to commit to tmfink:race-fix branch directly.
Others can open a Pull Request to this branch.

aquynh · 2018-06-20T18:14:23Z

looking again at this, here is my thoughts:

we dont really need Python script just to generate the content of cs_arch_globals.c, as this content is quite simple, and does not change over time.
what do you think about moving the content of this file, which is not big, to cs.c, so we dont need to modify msvc/xcode projects?

tmfink · 2018-06-20T19:17:36Z

@aquynh I can do that.

I originally decided not to paste the globals into cs.c because as we add more architectures, it reduces the likelihood of mistakes.

tmfink · 2018-06-20T23:07:22Z

I pushed the changes and updated the first comment.

aquynh · 2018-06-21T02:19:11Z

arch/AArch64/AArch64Module.h

@@ -0,0 +1,13 @@
+/* Capstone Disassembly Engine */
+/* By Nguyen Anh Quynh <aquynh@gmail.com>, 2018 */


now could you put your name here (and all other .h files), instead of mine?

Implemented. I should really learn my name and email at this point... 😀

aquynh · 2018-06-21T04:01:01Z

cs.c


-unsigned int all_arch = 0;
+unsigned int all_arch =


this will break if ARM is not enabled, because in that case we have all_arch = | ....

to fix this, we can put 0 in front of it (so all_arch = 0 | ...)

This actually works since the #else case puts in a 0.
The bitwise OR (|) operators are not inside the #ifdef ... #endif.

My build tests seem to confirm:

$ CAPSTONE_ARCHS=powerpc make -j CC cs.o CC utils.o CC SStream.o CC MCInstrDesc.o CC MCRegisterInfo.o CC arch/PowerPC/PPCInstPrinter.o CC arch/PowerPC/PPCDisassembler.o GEN capstone.pc CC arch/PowerPC/PPCMapping.o CC arch/PowerPC/PPCModule.o CC MCInst.o LINK libcapstone.so AR libcapstone.a ar: creating ./libcapstone.a make[1]: Entering directory 'capstone/cstool' CC cstool_x86.o CC cstool_arm64.o CC cstool_sparc.o CC cstool_ppc.o CC cstool_xcore.o CC cstool_arm.o CC cstool_systemz.o CC cstool_mips.o CC cstool.o LINK cstool make[1]: Leaving directory 'capstone/cstool' make -C tests make[1]: Entering directory 'capstone/tests' CC test_basic.o CC test_detail.o CC test_iter.o CC test_ppc.o CC test_skipdata.o LINK test_basic LINK test_detail LINK test_skipdata LINK test_iter LINK test_ppc LINK test_detail.static LINK test_iter.static LINK test_basic.static LINK test_skipdata.static LINK test_ppc.static make[1]: Leaving directory 'capstone/tests' install -m0755 ./libcapstone.so ./tests/ cd ./tests/ && mv libcapstone.so libcapstone.so.3 && ln -s libcapstone.so.3 libcapstone.so $ ./tests/test_ppc **************** Platform: PPC-64 Code:0x43 0x20 0x0c 0x07 0x41 0x56 0xff 0x17 0x80 0x20 0x00 0x00 0x80 0x3f 0x00 0x00 0x10 0x43 0x23 0x0e 0xd0 0x44 0x00 0x80 0x4c 0x43 0x22 0x02 0x2d 0x03 0x00 0x80 0x7c 0x43 0x20 0x14 0x7c 0x43 0x20 0x93 0x4f 0x20 0x00 0x21 0x4c 0xc8 0x00 0x21 0x40 0x82 0x00 0x14 Disasm: 0x1000: bdnzla+ 0xc04 op_count: 1 operands[0].type: IMM = 0xc04 Branch hint: 1 ...

oops you are right, i missed that 0

or without the #else

all_arch = 0 #ifdef CAPSTONE_HAS_ARM | (1 << CS_ARCH_ARM) #endif #ifdef CAPSTONE_HAS_ARM64 | (1 << CS_ARCH_ARM64) #endif ...

which saves a bit of space, not a big deal though.

yes i would do that too, but this is also OK

aquynh · 2018-06-21T04:01:37Z

cs.c

+#else
+	0
+#endif
+	;


please fix this indentation.

I adjusted the ; indent.
I purposely left the ; indented before but I suppose that it is clear already.

This eliminates the need for archs_enable() and eliminates the racey initialization. This makes the architecture-specific init, option, and destroy functions non-static so that they may be called from a different file.

tmfink · 2018-06-21T05:15:20Z

I considered doing that before but figured this would be easier to read. I did not want to special case the first architecture (by not using the OR operator) or use the '0' constant.

…

On Thu, Jun 21, 2018, 00:28 danghvu ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In cs.c <#1171 (comment)>: > -unsigned int all_arch = 0; +unsigned int all_arch = or without the #else all_arch = 0 #ifdef CAPSTONE_HAS_ARM | (1 << CS_ARCH_ARM) #endif #ifdef CAPSTONE_HAS_ARM64 | (1 << CS_ARCH_ARM64) #endif ... which saves a bit of space, not a big deal though. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1171 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBYuAJ1Ct9Td6t9K8ZZFyR-twSKk1Ykks5t-yDigaJpZM4UhKu9> .

aquynh · 2018-06-21T06:52:55Z

merged, thanks!

now could you port this to the next branch, too?

tmfink · 2018-06-21T12:19:42Z

Sure.

This eliminates the need for archs_enable() and eliminates the racey initialization. This makes the architecture-specific init and option functions non-static so that they may be called from a different file. Cherry-picked 853a287

* Declare global arch arrays with contents (#1171) This eliminates the need for archs_enable() and eliminates the racey initialization. This makes the architecture-specific init and option functions non-static so that they may be called from a different file. Cherry-picked 853a287 * Add cs_arch_disallowed_mode_mask global Cherry-pick 94bce43: mips: CS_MODE_MIPS32R6 implies CS_MODE_32 Cherry-pick 8998a3a: ppc: fix endian check (#1029) Fixes bug where endianness could not be set for ppc. Remove `big_endian` field of `cs_struct`. Added a helper macro `MODE_IS_BIG_ENDIAN()` to check if `CS_MODE_BIG_ENDIAN` is set. Refactored `cs_open()` check for valid mode out of arch-specific code into arch-independent code. Also added a valid mode check to `cs_option()`. The checks use a new global array `cs_arch_disallowed_mode_mask[]`. * Make global arrays static Make all_arch uint32_t to guarantee a certain number of bits (with adequate room for growth).

tmfink · 2018-07-02T14:45:10Z

FYI, the scripts I used to generate code:
https://gist.github.com/tmfink/de3683c1969c3d3b7b05b5fa141e3667

tmfink force-pushed the race-fix branch 2 times, most recently from e5df533 to e8941be Compare June 13, 2018 16:59

tmfink changed the title ~~Add CAPSTONE_THREAD_SAFE to enable thread safety~~ Eliminates run-time initialization of global variables (fixes race condition) Jun 13, 2018

aquynh reviewed Jun 13, 2018

View changes

tmfink force-pushed the race-fix branch from e8941be to 1176e4e Compare June 14, 2018 02:45

tmfink force-pushed the race-fix branch from 1176e4e to 97fba0b Compare June 20, 2018 22:56

aquynh reviewed Jun 21, 2018

View changes

tmfink force-pushed the race-fix branch from 97fba0b to 3dba65a Compare June 21, 2018 03:44

aquynh reviewed Jun 21, 2018

View changes

Declare global arch arrays with contents

64d7b9b

This eliminates the need for archs_enable() and eliminates the racey initialization. This makes the architecture-specific init, option, and destroy functions non-static so that they may be called from a different file.

tmfink force-pushed the race-fix branch from 3dba65a to 64d7b9b Compare June 21, 2018 04:23

aquynh merged commit 853a287 into capstone-engine:master Jun 21, 2018

tmfink mentioned this pull request Feb 26, 2019

m68k: data race initializing global g_instruction_table in build_opcode_table() #1397

Closed

tmfink mentioned this pull request Feb 18, 2023

Add option to build frida capstone bindings from git capstone-rust/capstone-rs#140

Closed

		@@ -0,0 +1,13 @@
		/* Capstone Disassembly Engine */
		/* By Nguyen Anh Quynh <aquynh@gmail.com>, 2018 */

+              #else
+
+              #endif
+              	;

Eliminates run-time initialization of global variables (fixes race condition) #1171

Eliminates run-time initialization of global variables (fixes race condition) #1171

Conversation

tmfink commented Jun 9, 2018 • edited

danghvu commented Jun 9, 2018 • edited

aquynh commented Jun 9, 2018 via email

tmfink commented Jun 9, 2018 • edited

tmfink commented Jun 9, 2018

tmfink commented Jun 9, 2018

tmfink commented Jun 9, 2018

danghvu commented Jun 9, 2018

tmfink commented Jun 11, 2018

tmfink commented Jun 13, 2018

danghvu commented Jun 13, 2018

radare commented Jun 13, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmfink commented Jun 14, 2018

stevemk14ebr commented Jun 15, 2018 • edited

danghvu commented Jun 15, 2018

stevemk14ebr commented Jun 15, 2018 • edited

danghvu commented Jun 15, 2018

stevemk14ebr commented Jun 15, 2018 • edited

danghvu commented Jun 15, 2018 • edited

tmfink commented Jun 15, 2018 • edited

tmfink commented Jun 15, 2018

aquynh commented Jun 15, 2018

aquynh commented Jun 20, 2018

tmfink commented Jun 20, 2018

aquynh commented Jun 20, 2018

tmfink commented Jun 20, 2018

tmfink commented Jun 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aquynh Jun 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmfink commented Jun 21, 2018 via email

aquynh commented Jun 21, 2018

tmfink commented Jun 21, 2018

tmfink commented Jul 2, 2018

tmfink commented Jun 9, 2018 •

edited

danghvu commented Jun 9, 2018 •

edited

tmfink commented Jun 9, 2018 •

edited

stevemk14ebr commented Jun 15, 2018 •

edited

stevemk14ebr commented Jun 15, 2018 •

edited

stevemk14ebr commented Jun 15, 2018 •

edited

danghvu commented Jun 15, 2018 •

edited

tmfink commented Jun 15, 2018 •

edited

aquynh Jun 21, 2018 •

edited