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

cynara-test depends on random initialization of global instances #9

Open
pohly opened this issue May 29, 2015 · 5 comments
Open

cynara-test depends on random initialization of global instances #9

pohly opened this issue May 29, 2015 · 5 comments

Comments

@pohly
Copy link
Contributor

pohly commented May 29, 2015

When cross-compiled with gcc 4.9 using the meta-framework-security bitbake recipes [1], cynara-test immediately segfaults before even reaching main(). I suspect that PolicyKeyFeature::m_wildcardValue (a global instance) when k_nun (another global) gets created (_M_p = 0x0 in m_wildcardValue).

Program received signal SIGSEGV, Segmentation fault.
0x080c0f94 in operator== (__rhs=..., __lhs=...)
at /work/build/iot/x86/tmp-glibc/sysroots/qemux86/usr/include/c++/4.9.2/bits/basic_string.h:2516
2516 /work/build/iot/x86/tmp-glibc/sysroots/qemux86/usr/include/c++/4.9.2/bits/basic_string.h: No such file or directory.
(gdb) where
#0 0x080c0f94 in operator== (__rhs=..., __lhs=...)

at /work/build/iot/x86/tmp-glibc/sysroots/qemux86/usr/include/c++/4.9.2/bits/basic_string.h:2516

#1 Cynara::PolicyKeyFeature::PolicyKeyFeature (this=0xbffffcb8, value=...)

at /usr/src/debug/cynara/0.8.0+gitAUTOINC+b4cb3d8e86-r0/git/src/common/types/PolicyKey.h:89

#2 0x08070a55 in create (value=...)

at /usr/src/debug/cynara/0.8.0+gitAUTOINC+b4cb3d8e86-r0/git/src/common/types/PolicyKey.h:48

#3 __static_initialization_and_destruction_0 (__priority=65535,

__initialize_p=1)
at /usr/src/debug/cynara/0.8.0+gitAUTOINC+b4cb3d8e86-r0/git/test/common/protocols/TestDataCollection.h:40

#4 0x081e2ae4 in __do_global_ctors_aux ()
#5 0x0805ebc0 in _init ()
#6 0x494d8b0c in ?? () from /usr/lib/libstdc++.so.6

Backtrace stopped: previous frame inner to this frame (corrupt stack?)
(gdb) up
#1 Cynara::PolicyKeyFeature::PolicyKeyFeature (this=0xbffffcb8, value=...)

at /usr/src/debug/cynara/0.8.0+gitAUTOINC+b4cb3d8e86-r0/git/src/common/types/PolicyKey.h:89

89 m_isWildcard(value == PolicyKeyFeature::m_wildcardValue),
(gdb) p value
$1 = (const Cynara::PolicyKeyFeature::ValueType &) @0xbffffca4: {
static npos = ,
_M_dataplus = {std::allocator = {<__gnu_cxx::new_allocator> = {}, }, _M_p = 0x827ee8c ""}}
(gdb) up
#2 0x08070a55 in create (value=...)

at /usr/src/debug/cynara/0.8.0+gitAUTOINC+b4cb3d8e86-r0/git/src/common/types/PolicyKey.h:48

48 return PolicyKeyFeature(value);
(gdb) up
#3 __static_initialization_and_destruction_0 (__priority=65535,

__initialize_p=1)
at /usr/src/debug/cynara/0.8.0+gitAUTOINC+b4cb3d8e86-r0/git/test/common/protocols/TestDataCollection.h:40

40 static const Cynara::PolicyKey k_nun(PKF::create(""), PKF::create("u"), PKF::create(""));
(gdb) p PolicyKeyFeature::m_wildcardValue
$1 = {static npos = ,
_M_dataplus = {std::allocator = {<__gnu_cxx::new_allocator> = {}, }, _M_p = 0x0}}

[1] https://github.com/01org/meta-intel-iot-security (recipe with cynara-test enabled to be published there soon)

@pohly
Copy link
Contributor Author

pohly commented May 29, 2015

m_wildcardValue was just the tip of the iceberg. Several other global instances had similar issues. Some of that code is also used in cynarad, so this issue might go beyond just testing.

For the fixes that were good enough for me at the moment, see:
https://github.com/pohly/meta-intel-iot-security/blob/cynara-tests/meta-security-framework/recipes-security/cynara/cynara/PolicyKeyFeature-avoid-complex-global-constants.patch
https://github.com/pohly/meta-intel-iot-security/blob/cynara-tests/meta-security-framework/recipes-security/cynara/cynara/globals-avoid-copying-other-globals.patch

However, you might want to do a more thorough code review and avoid this kind of issue completely. I only fixed the cases that really caused segfaults.

@JacquesBurac
Copy link
Contributor

Thanks for finding these issues and for the patches. Yes, we need to get rid of direct std::string -> std::string dependencies. First patch looks good to me.
The second patch if I understand static variable initialization correctly makes it less likely to fail, because there are less variables that we depend on. However, we still rely on variable being referenced to be initialized first (references can point to variable that have not yet been initialized).
I think that all std::string constants should depend on:
a) C string literals/variables
or
b) wrapper functions like in the first patch

@pohly
Copy link
Contributor Author

pohly commented Jun 2, 2015

The second patch if I understand static variable initialization correctly makes it less likely to fail,
because there are less variables that we depend on. However, we still rely on variable being
referenced to be initialized first (references can point to variable that have not yet been initialized).

Correct. It's just mitigating the problem without solving it properly.

I agree with the proposed solution, but won't have time to implement it, I'm afraid.

@JacquesBurac
Copy link
Contributor

Ok, I understand. Right now I'm working on some urgent task, but I should finish soon and I'll be able to take care of it.

@JacquesBurac
Copy link
Contributor

I've pushed your first patch to gerrit:
https://review.tizen.org/gerrit/#/c/41353

And also made some more changes:
https://review.tizen.org/gerrit/#/c/41354

pohly added a commit to pohly/cynara that referenced this issue Sep 16, 2015
PolicyKeyFeature is used by other global instances in cynara-test
and cannot assume that the initialization of its own static constants
happens first, unless it enforces initialization by embedding
these constants in method calls.

Upstream-status: Submitted [Samsung#9]

Signed-off-by: Patrick Ohly <[email protected]>

%% original patch: PolicyKeyFeature-avoid-complex-global-constants.patch
pohly added a commit to pohly/cynara that referenced this issue Sep 16, 2015
In several places, a global constant is initialized with the constant
of another constant of the same type. This only works if these
instances happen to be initialized in the right order, which is not
guaranteed and indeed happened to fail when using gcc 4.9 (see
Samsung#9).

The solution used here replaces the copies of the other constants with
references to them, which minimizes source code changes and is also
slightly more efficient. The references can be created at any time
because the address of the real constants cannot change.

This change was enough to get cynara-tests running; it is not necessarily
complete.

Upstream-status: Submitted [Samsung#9]

Signed-off-by: Patrick Ohly <[email protected]>

%% original patch: globals-avoid-copying-other-globals.patch
lukaszwojciechowski pushed a commit that referenced this issue Sep 25, 2015
PolicyKeyFeature is used by other global instances in cynara-test
and cannot assume that the initialization of its own static constants
happens first, unless it enforces initialization by embedding
these constants in method calls.

Upstream-status: Submitted [#9]
Change-Id: Ifa6dcd44ce059cf3ec8c99764bd6ea0c677cdd6d
Signed-off-by: Patrick Ohly <[email protected]>
Signed-off-by: Radoslaw Bartosiak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants