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

tmfink · 2019-02-26T06:14:57Z

The global array g_instruction_table init in M680XDisassembler.c:build_opcode_table() is a race condition.

Declaration:
https://github.com/aquynh/capstone/blob/de952a3e5a519b4a0d0dd2ef921a755891860ca6/arch/M68K/M68KDisassembler.c#L250-L251

Initialization:
https://github.com/aquynh/capstone/blob/de952a3e5a519b4a0d0dd2ef921a755891860ca6/arch/M68K/M68KDisassembler.c#L3787-L3827

This caused problems while working on the Rust language bindings. The tests are run in parallel:
- PR: capstone-rust/capstone-rs#60
- Travis CI failure: https://travis-ci.org/capstone-rust/capstone-rs/jobs/498529801#L623

Possible solutions:

Introduce synchronization in build_opcode_table()
- Would require adding a threading library dependency (like pthread)
Declare array with appropriate values statically
- Most efficient (run time) method
- Does not add extra dependencies or change API
- Requires deeper knowledge of m68k code
- May require writing a script to generate C declaration
Introduce capstone_init() function that must be called before any other Capstone functions
- API breaking change
- Less ergonomic for users
Move the global variable into the private cs_struct
- Less efficient: wastes time initializing and memory

Similar past issue: #1171

The text was updated successfully, but these errors were encountered:

aquynh · 2019-02-26T06:28:46Z

@emoon, i think the solution (2) is the best.

radare · 2019-02-26T06:40:41Z

Imho a library initializer/constructor would be cleaner but that will break backwards compat.

…

On 26 Feb 2019, at 07:28, Nguyen Anh Quynh ***@***.***> wrote: @emoon, i think the solution (2) is the best. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

emoon · 2019-02-26T09:22:58Z

I would agree with @radare that a init function would be cleaner but (2) wouldn't break anything so I guess is what needs to be done. I guess what I need to do is to write a script that generates the table. I can include the script as an optional thing that people that changes the code may need to use. Users in general will never need to poke around in this code so having a manual step isn't the biggest issue I would say and in many cases one wouldn't need to change this anyway.

I can try to look at this during the week and hopefully today.

aquynh · 2019-02-26T09:41:00Z

I would agree with @radare that a init function would be cleaner but (2) wouldn't break anything so I guess is what needs to be done. I guess what I need to do is to write a script that generates the table. I can include the script as an optional thing that people that changes the code may need to use. Users in general will never need to poke around in this code so having a manual step isn't the biggest issue I would say and in many cases one wouldn't need to change this anyway.

m68k is a stable architecture, without any future update (?), so this is one time change, and should work well.

emoon · 2019-02-26T09:49:51Z

Yeah I think it will be fine.

tmfink · 2019-02-26T12:14:49Z

@emoon definitely include the script that is used to generate the array. That will make future contributions easier. :-)

emoon · 2019-02-26T12:49:11Z

@tmfink yes, that is the plan :)

radare · 2019-02-26T12:56:59Z

Overcomplicating the implementwtion of a library messing with threads, mutexes, locks and such when it just needs a decent api is a bad idea imho. Because the null chk in the fini call as well as the handler resolution instead of just using a pointer holding the initialized library would be other changes i would appreciate to see in the next release if you are in the mood to change the api

…

On 26 Feb 2019, at 10:41, Nguyen Anh Quynh ***@***.***> wrote: I would agree with @radare that a init function would be cleaner but (2) wouldn't break anything so I guess is what needs to be done. I guess what I need to do is to write a script that generates the table. I can include the script as an optional thing that people that changes the code may need to use. Users in general will never need to poke around in this code so having a manual step isn't the biggest issue I would say and in many cases one wouldn't need to change this anyway. m68k is a stable architecture, without any future update (?), so this is one time change, and should work well. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

emoon · 2019-02-26T17:11:14Z

Well the fix in this case won't involve any threading setup but I agree it would be nice with an init call but I will do the pre-create table approach here.

emoon · 2019-02-27T11:25:47Z

It's important to realize with this change there will be a file with ~65535 lines of code in it. While no compilers these days will have any issues with (esp as it's data only) it will likely bump the compile times a bit.

tmfink · 2019-02-27T16:41:47Z

We may want such a large file to be #include'd instead of dropped in directly.

…

On Wed, Feb 27, 2019, 06:25 Daniel Collin ***@***.***> wrote: It's important to realize with this change there will be a file with ~65535 lines of code in it. While no compilers these days will have any issues with (esp as it's data only) it will likely bump the compile times a bit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1397 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFBYuFAa97RIxwMBiHF6CAqAiqqepM4Nks5vRmtDgaJpZM4bRb5n> .

emoon · 2019-02-27T16:45:53Z

Sure it can be an include.

radare · 2019-02-27T17:16:00Z

That will probably hurt in the final binary size. And capstone is already quite big...

…

On 27 Feb 2019, at 17:45, Daniel Collin ***@***.***> wrote: Sure it can be an include. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

aquynh · 2019-02-27T17:16:03Z

Including big files is what we have been doing for x86 & other LLVM based archs. And 64k files are nothing, no worry :-)

aquynh · 2019-02-27T17:25:12Z

Unfortunately we have to choose between performance & size. For now, most efforts are to make the core faster. For X86, i have been doing many optimization on the binary size, because people want to put Capstone inside OS kernels or so. There is no such demand for other archs. Of course there are other options available, such as selected build only archs you need, or the diet mode.

emoon · 2019-02-28T05:35:44Z

Alright. I will implement this on Friday. Expect a PR then

fanfuqiang · 2019-03-01T11:06:11Z

Why not add a C11 atomic flag?

typedef struct {

    void (*instruction)(m68k_info *info);    /* handler function */

     _Atomic _Bool is_init;         /* C11 atomic type */

    uint word2_mask;              /* mask the 2nd word */

    uint word2_match;             /* what to match after masking */

instruction_struct;

aquynh · 2019-03-01T11:08:16Z

Because we want to support all compilers?

fanfuqiang · 2019-03-01T11:14:35Z

Because we want to support all compilers?

It's part of standard C11 language, all platforms, and compilers can support.

aquynh · 2019-03-01T11:19:16Z

Even older compiler versions?

fanfuqiang · 2019-03-01T11:24:15Z

Even older compiler versions?

Well, maybe not.
compilers released after 2011 should be works.

emoon · 2019-03-01T11:42:34Z

I can use the C11 atomics for this yes if @aquynh think it's acceptable to require a C11 compatible compiler. Also there is no need to have is_init per entry. It's possible to rewrite the init loop instead and have several threads working on setting up the array with just one atomic counter.

aquynh · 2019-03-01T11:45:39Z

No please. You never know which compilers users are using, so better if we dont force them. You will not believe, but i had to use 20+ years old compiler, and i had no choice.

emoon · 2019-03-01T11:48:47Z

Ouch :( ok, well I have a PR for the static table anyway.

tmfink · 2019-03-01T19:02:17Z

Other issue with using atomics:

It's slower/more complicated than just having the table defined statically
There could be complications with calling into the C code from other language (such as Rust) where the caller needs to obey some extra logic
C11 means the standard was defined in 2011; compilers can be slow to adopt the standard. Different compilers may also only support parts of the standard.

emoon · 2019-03-01T19:07:18Z

To be fair having the table static has it's issue also:

Slower compile times
More memory usage during compilation
Larger source code

Also the calling code from the outside would be identical when using atomic so that wouldn't be much of an issue. Actually the creation of the table would be faster because multiple threads would do it at the same time but that it's not really a big issue as it's only being done once.

But as written above atomics won't be used anyway and I have the PR ready for the static table anyway.

tmfink · 2019-03-01T19:17:41Z

@emoon Can we redefine instruction_struct to use smaller types (such as uint16_t) instead of uint?

https://github.com/aquynh/capstone/blob/de952a3e5a519b4a0d0dd2ef921a755891860ca6/arch/M68K/M68KDisassembler.c#L240-L244

Also, while you are already modifying the file, can you eliminate the uint #define? The width can change on different architectures. If we only need 32 bits, then we should say uint32_t.
https://github.com/aquynh/capstone/blob/de952a3e5a519b4a0d0dd2ef921a755891860ca6/arch/M68K/M68KDisassembler.c#L62-L64

Thanks for your work!

emoon · 2019-03-01T19:21:54Z

Sure I can fix that and thanks :) (edit: This is now fixed in the PR)

emoon · 2019-03-04T17:57:05Z

With #1408 being merged can this be closed now?

aquynh · 2019-03-05T02:21:27Z

close, thanks!

tmfink mentioned this issue Mar 1, 2019

Support Capstone 4.0 WIP capstone-rust/capstone-rs#60

Merged

23 tasks

emoon mentioned this issue Mar 1, 2019

[M68K] Made instruction table static #1408

Merged

aquynh closed this as completed Mar 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tmfink commented Feb 26, 2019

aquynh commented Feb 26, 2019

radare commented Feb 26, 2019 via email

emoon commented Feb 26, 2019

aquynh commented Feb 26, 2019

emoon commented Feb 26, 2019

tmfink commented Feb 26, 2019

emoon commented Feb 26, 2019

radare commented Feb 26, 2019 via email

emoon commented Feb 26, 2019

emoon commented Feb 27, 2019

tmfink commented Feb 27, 2019 via email

emoon commented Feb 27, 2019

radare commented Feb 27, 2019 via email

aquynh commented Feb 27, 2019 via email

aquynh commented Feb 27, 2019 via email

emoon commented Feb 28, 2019

fanfuqiang commented Mar 1, 2019 •

edited

aquynh commented Mar 1, 2019 via email

fanfuqiang commented Mar 1, 2019

aquynh commented Mar 1, 2019 via email

fanfuqiang commented Mar 1, 2019

emoon commented Mar 1, 2019

aquynh commented Mar 1, 2019 via email

emoon commented Mar 1, 2019

tmfink commented Mar 1, 2019

emoon commented Mar 1, 2019 •

edited

tmfink commented Mar 1, 2019

emoon commented Mar 1, 2019 •

edited

emoon commented Mar 4, 2019

aquynh commented Mar 5, 2019

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

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

Comments

tmfink commented Feb 26, 2019

aquynh commented Feb 26, 2019

radare commented Feb 26, 2019 via email

emoon commented Feb 26, 2019

aquynh commented Feb 26, 2019

emoon commented Feb 26, 2019

tmfink commented Feb 26, 2019

emoon commented Feb 26, 2019

radare commented Feb 26, 2019 via email

emoon commented Feb 26, 2019

emoon commented Feb 27, 2019

tmfink commented Feb 27, 2019 via email

emoon commented Feb 27, 2019

radare commented Feb 27, 2019 via email

aquynh commented Feb 27, 2019 via email

aquynh commented Feb 27, 2019 via email

emoon commented Feb 28, 2019

fanfuqiang commented Mar 1, 2019 • edited

aquynh commented Mar 1, 2019 via email

fanfuqiang commented Mar 1, 2019

aquynh commented Mar 1, 2019 via email

fanfuqiang commented Mar 1, 2019

emoon commented Mar 1, 2019

aquynh commented Mar 1, 2019 via email

emoon commented Mar 1, 2019

tmfink commented Mar 1, 2019

emoon commented Mar 1, 2019 • edited

tmfink commented Mar 1, 2019

emoon commented Mar 1, 2019 • edited

emoon commented Mar 4, 2019

aquynh commented Mar 5, 2019

fanfuqiang commented Mar 1, 2019 •

edited

emoon commented Mar 1, 2019 •

edited

emoon commented Mar 1, 2019 •

edited