Critical section verification

[inconstante]

Some functions in tools/introspection.c are marked as belonging to a critical section. This means that they should only be used after all threads have been properly hijacked (i.e. attached to and stopped).

However, Libpulp's tools do not check that before executing them (we simply rely on the fact that we also write the tools, so we know what we are doing (famous last words)). This check should be added to Libpulp.

Implementation-wise, a new struct member in struct ulp_process (see tools/introspection.h) could do the trick. After hijacking (i.e. at the end of hijack_threads), this flag would be set; then it's only a matter of checking this flag at the start of every function that requires critical-section guarantees; finally, on restore_threads the flag should be reset.

[a-gavin]

Happy to work on this. When checking at the beginning of a critical section function, should a warning be output in the case that not all threads were hijacked? If so, is "not all threads hijacked" sufficient?

[inconstante]

When checking at the beginning of a critical section function, should a warning be output in the case that not all threads were hijacked?

We have lots of warnings everywhere, so yes, it should be fine to add yet another one.

It should also not actually call into libpulp.so, i.e.: it should refrain from calling run_and_redirect.

If so, is "not all threads hijacked" sufficient?

Sounds good to me, yes.

[a-gavin]

It should also not actually call into libpulp.so, i.e.: it should refrain from calling run_and_redirect.

Sounds good.

I've been attempting to write a test case that covers some of this change but have run into some difficulty with ptrace. In the test case, ulp_trigger should be able to attach to the main thread of a program but not a child thread. My intention is to demonstrate expected failure entering the critical section in hijack_threads() when the function attempts to attach to all threads of a process.

Unfortunately, I have been unable to do so...yet? I've read through the ptrace man page several times and am unable to determine whether some processes A and B can attach to some process Z's two threads separately. According to the man page, it should be possible:

Attachment and subsequent commands are per thread: in a multithreaded process, every thread can be individually attached to a (potentially different) tracer, or left not attached and thus not debugged.

This is my first time working with ptrace, so I'd just like to verify that this is even possible before I continue debugging the test case. Test case aside, I believe I have written necessary checking code for this in my fork. Always good to have some testing for changes, though!

[inconstante]

I've been attempting to write a test case that covers some of this change but have run into some difficulty with ptrace. In the test case, ulp_trigger should be able to attach to the main thread of a program but not a child thread. My intention is to demonstrate expected failure entering the critical section in hijack_threads() when the function attempts to attach to all threads of a process.

That's clever. I think it should work.

Unfortunately, I have been unable to do so...yet?

Alternatively, you could create a dummy tool, very similar to ulp_trigger, that calls apply_patch, but doesn't call hijack_threads beforehand.

This is my first time working with ptrace, so I'd just like to verify that this is even possible before I continue debugging the test case.

It should be possible, indeed.

Test case aside, I believe I have written necessary checking code for this in my fork. Always good to have some testing for changes, though!

I'd suggest that you go ahead submit what you have before trying to write a new test case so that we can have a look. I would be OK with a patch that doesn't contain a test case (at least in this case. :)).

[a-gavin]

Alternatively, you could create a dummy tool, very similar to ulp_trigger, that calls apply_patch, but doesn't call hijack_threads beforehand.

That's a good idea. If I am unable to get the ptrace approach to work, I will use that approach instead.

Test case aside, I believe I have written necessary checking code for this in my fork. Always good to have some testing for changes, though!

I'd suggest that you go ahead submit what you have before trying to write a new test case so that we can have a look. I would be OK with a patch that doesn't contain a test case (at least in this case. :)).

Sounds good.

[a-gavin]

Removed reference to GitHub Issue number in commit and rebased on to current upstream master branch for PR #36. Fully ready for review now.

[giulianobelinassi]

Closed in b77f2761fa92de3d6e7d6ab2b901943fe623707e