Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Jun 21, 2018

Conversation

@tmfink
Copy link
Contributor

@tmfink tmfink commented Jun 9, 2018

Declare global arch arrays with contents.

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

Fixes #1168.

Progress:

  • declare global arrays with values
@danghvu
Copy link
Collaborator

@danghvu danghvu commented Jun 9, 2018

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
Copy link
Owner

@aquynh aquynh commented Jun 9, 2018

@tmfink
Copy link
Contributor Author

@tmfink tmfink commented Jun 9, 2018

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
Copy link
Contributor Author

@tmfink tmfink commented Jun 9, 2018

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
Copy link
Contributor Author

@tmfink tmfink commented Jun 9, 2018

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

@tmfink
Copy link
Contributor Author

@tmfink tmfink commented Jun 9, 2018

@danghvu
Copy link
Collaborator

@danghvu danghvu commented Jun 9, 2018

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
Copy link
Contributor Author

@tmfink tmfink commented Jun 11, 2018

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.

  1. 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.
  2. Just about any libpthread implementation should allow dynamic linking.
@tmfink tmfink force-pushed the tmfink:race-fix branch 2 times, most recently from e5df533 to e8941be Jun 13, 2018
@tmfink
Copy link
Contributor Author

@tmfink tmfink commented Jun 13, 2018

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

@tmfink 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
@danghvu
Copy link
Collaborator

@danghvu danghvu commented Jun 13, 2018

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
Copy link
Contributor

@radare radare commented Jun 13, 2018


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

This comment has been minimized.

@aquynh

aquynh Jun 13, 2018
Owner

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.

This comment has been minimized.

@tmfink

tmfink Jun 14, 2018
Author Contributor

Thanks, done.

@tmfink tmfink force-pushed the tmfink:race-fix branch from e8941be to 1176e4e Jun 14, 2018
@tmfink
Copy link
Contributor Author

@tmfink tmfink commented Jun 14, 2018

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

@stevemk14ebr
Copy link
Contributor

@stevemk14ebr stevemk14ebr commented Jun 15, 2018

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
Copy link
Collaborator

@danghvu danghvu commented Jun 15, 2018

@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
Copy link
Contributor

@stevemk14ebr stevemk14ebr commented Jun 15, 2018

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
Copy link
Collaborator

@danghvu danghvu commented Jun 15, 2018

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
Copy link
Contributor

@stevemk14ebr stevemk14ebr commented Jun 15, 2018

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
Copy link
Collaborator

@danghvu danghvu commented Jun 15, 2018

@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
Copy link
Contributor Author

@tmfink tmfink commented Jun 15, 2018

@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
Copy link
Contributor Author

@tmfink tmfink commented Jun 15, 2018

@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
Copy link
Owner

@aquynh aquynh commented Jun 15, 2018

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
Copy link
Owner

@aquynh aquynh commented Jun 20, 2018

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
Copy link
Contributor Author

@tmfink tmfink commented Jun 20, 2018

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
Copy link
Owner

@aquynh aquynh commented Jun 20, 2018

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
Copy link
Contributor Author

@tmfink tmfink commented Jun 20, 2018

@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 tmfink force-pushed the tmfink:race-fix branch from 1176e4e to 97fba0b Jun 20, 2018
@tmfink
Copy link
Contributor Author

@tmfink tmfink commented Jun 20, 2018

I pushed the changes and updated the first comment.

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

This comment has been minimized.

@aquynh

aquynh Jun 21, 2018
Owner

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

This comment has been minimized.

@tmfink

tmfink Jun 21, 2018
Author Contributor

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

@tmfink tmfink force-pushed the tmfink:race-fix branch from 97fba0b to 3dba65a Jun 21, 2018

unsigned int all_arch = 0;
unsigned int all_arch =

This comment has been minimized.

@aquynh

aquynh Jun 21, 2018
Owner

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 comment has been minimized.

@tmfink

tmfink Jun 21, 2018
Author Contributor

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
...

This comment has been minimized.

@aquynh

aquynh Jun 21, 2018
Owner

oops you are right, i missed that 0

This comment has been minimized.

@danghvu

danghvu Jun 21, 2018
Collaborator

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.

This comment has been minimized.

@aquynh

aquynh Jun 21, 2018
Owner

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

cs.c Outdated
#else
0
#endif
;

This comment has been minimized.

@aquynh

aquynh Jun 21, 2018
Owner

please fix this indentation.

This comment has been minimized.

@tmfink

tmfink Jun 21, 2018
Author Contributor

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 tmfink force-pushed the tmfink:race-fix branch from 3dba65a to 64d7b9b Jun 21, 2018
@tmfink
Copy link
Contributor Author

@tmfink tmfink commented Jun 21, 2018

@aquynh aquynh merged commit 853a287 into aquynh:master Jun 21, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aquynh
Copy link
Owner

@aquynh aquynh commented Jun 21, 2018

merged, thanks!

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

@tmfink
Copy link
Contributor Author

@tmfink tmfink commented Jun 21, 2018

Sure.

tmfink added a commit to tmfink/capstone that referenced this pull request Jun 21, 2018
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
aquynh added a commit that referenced this pull request Jun 24, 2018
* 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
Copy link
Contributor Author

@tmfink tmfink commented Jul 2, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.