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

fix(libtor): prevent concurrent instances #1445

Merged
merged 11 commits into from
Jan 16, 2024
Merged

fix(libtor): prevent concurrent instances #1445

merged 11 commits into from
Jan 16, 2024

Conversation

bassosimone
Copy link
Contributor

@bassosimone bassosimone commented Jan 15, 2024

This PR is about preventing concurrent ./internal/libtor instances. The ./internal/libtor package links with libtor.a and runs tor_run_main. The expectation of such a function is that a single thread can execute within the same process. If we attempt to invoke tor_run_main while another instance is running, this is most likely going to cause memory corruption because the two instances try to operate on the same static data structures.

To address this issue, we use atomic compare and swap and atomic set to make sure there is just a single thread inside the "critical section" consisting of calling tor_run_main. All the other threads would fail with an error.

To test this branch on Linux, you need to run:

go run ./internal/cmd/buildtool linux cdeps zlib openssl libevent tor

to compile tor and its dependencies for GNU/Linux. (You would need build-essential, autoconf, automake, and libtool being installed on a Debian system.)

Then, run tests using:

go test -tags ooni_libtor -v ./internal/libtor/...

These tests should pass on a GNU/Linux system.

In addition to making sure we cannot have concurrent instances, this PR also modifies some tests that were not using SocksPort auto. As a result, those tests would fail in a system where tor is running as a daemon.

Closes ooni/probe#2623

@bassosimone bassosimone requested a review from hellais as a code owner January 15, 2024 17:41
@bassosimone
Copy link
Contributor Author

bassosimone commented Jan 15, 2024

Tests for libtorlinux passed in https://github.com/ooni/probe-cli/actions/runs/7532480622/job/20503178535, which was building 376a8f5. So, now I can revert the little piece of GitHub Action changes that caused the build to trigger.

@ainghazal
Copy link
Contributor

I failed to run the test referenced above, in an arch-based system. Sounds like the build tool is missing to link against libcap?

❯ go test -tags ooni_libtor -v ./internal/libtor/...
# github.com/ooni/probe-cli/v3/internal/libtor.test
/home/admin/sdk/go1.20.13/pkg/tool/linux_amd64/link: running gcc failed: exit status 1
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(sandbox.o): in function `sb_kill':
sandbox.c:(.text+0x23f): undefined reference to `seccomp_rule_add'
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(sandbox.o): in function `sb_ioctl':
sandbox.c:(.text+0x2c3): undefined reference to `seccomp_rule_add'
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(sandbox.o): in function `sb_socketpair':
sandbox.c:(.text+0x371): undefined reference to `seccomp_rule_add'
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(sandbox.o): in function `sb_mremap':
sandbox.c:(.text+0x3f3): undefined reference to `seccomp_rule_add'
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(sandbox.o): in function `sb_prctl':
sandbox.c:(.text+0x473): undefined reference to `seccomp_rule_add'
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(sandbox.o):sandbox.c:(.text+0x5e8): more undefined references to `seccomp_rule_add' follow
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(sandbox.o): in function `sandbox_init':
sandbox.c:(.text+0x2902): undefined reference to `seccomp_init'
/usr/bin/ld: sandbox.c:(.text+0x2a2e): undefined reference to `seccomp_release'
/usr/bin/ld: sandbox.c:(.text+0x2ade): undefined reference to `seccomp_rule_add'
/usr/bin/ld: sandbox.c:(.text+0x2b41): undefined reference to `seccomp_rule_add'
/usr/bin/ld: sandbox.c:(.text+0x2c1c): undefined reference to `seccomp_rule_add'
/usr/bin/ld: sandbox.c:(.text+0x2cee): undefined reference to `seccomp_rule_add'
/usr/bin/ld: sandbox.c:(.text+0x2d8d): undefined reference to `seccomp_release'
/usr/bin/ld: sandbox.c:(.text+0x2de2): undefined reference to `seccomp_release'
/usr/bin/ld: sandbox.c:(.text+0x2e28): undefined reference to `seccomp_rule_add'
/usr/bin/ld: sandbox.c:(.text+0x2e8a): undefined reference to `seccomp_load'
/usr/bin/ld: sandbox.c:(.text+0x2ea6): undefined reference to `seccomp_release'
/usr/bin/ld: sandbox.c:(.text+0x2ef6): undefined reference to `seccomp_rule_add'
/usr/bin/ld: sandbox.c:(.text+0x2f57): undefined reference to `seccomp_release'
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(setuid.o): in function `drop_capabilities':
setuid.c:(.text+0x64): undefined reference to `cap_get_proc'
/usr/bin/ld: setuid.c:(.text+0x7d): undefined reference to `cap_clear'
/usr/bin/ld: setuid.c:(.text+0x92): undefined reference to `cap_set_flag'
/usr/bin/ld: setuid.c:(.text+0xaa): undefined reference to `cap_set_flag'
/usr/bin/ld: setuid.c:(.text+0xc5): undefined reference to `cap_set_flag'
/usr/bin/ld: setuid.c:(.text+0xcd): undefined reference to `cap_set_proc'
/usr/bin/ld: setuid.c:(.text+0xd7): undefined reference to `cap_free'
/usr/bin/ld: /home/admin/ooni/probe-cli/internal/libtor/linux/amd64/lib/libtor.a(setuid.o): in function `have_capability_support':
setuid.c:(.text+0x415): undefined reference to `cap_get_proc'
/usr/bin/ld: setuid.c:(.text+0x424): undefined reference to `cap_free'
collect2: error: ld returned 1 exit status

FAIL    github.com/ooni/probe-cli/v3/internal/libtor [build failed]
FAIL

@bassosimone
Copy link
Contributor Author

I failed to run the test referenced above, in an arch-based system. Sounds like the build tool is missing to link against libcap?

Yes, it seems the ./configure or tor is picking up -lcap and -lseccomp if they are available. The easiest fix would be to pass flags to ./configure to disable using those libraries when building for GNU/Linux.

@bassosimone
Copy link
Contributor Author

@ainghazal I summarized our discussion here: ooni/probe#2651.

@bassosimone bassosimone merged commit a62f36c into master Jan 16, 2024
9 of 11 checks passed
@bassosimone bassosimone deleted the issue/2623 branch January 16, 2024 08:55
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this pull request Feb 13, 2024
This PR is about preventing concurrent `./internal/libtor` instances.
The `./internal/libtor` package links with `libtor.a` and runs
`tor_run_main`. The expectation of such a function is that a single
thread can execute within the same process. If we attempt to invoke
`tor_run_main` while another instance is running, this is most likely
going to cause memory corruption because the two instances try to
operate on the same static data structures.

To address this issue, we use atomic compare and swap and atomic set to
make sure there is just a single thread inside the "critical section"
consisting of calling `tor_run_main`. All the other threads would fail
with an error.

To test this branch on Linux, you need to run:

```sh
go run ./internal/cmd/buildtool linux cdeps zlib openssl libevent tor
```

to compile tor and its dependencies for GNU/Linux. (You would need
`build-essential`, `autoconf`, `automake`, and `libtool` being installed
on a Debian system.)

Then, run tests using:

```sh
go test -tags ooni_libtor -v ./internal/libtor/...
```

These tests should pass on a GNU/Linux system.

In addition to making sure we cannot have concurrent instances, this PR
also modifies some tests that were not using `SocksPort auto`. As a
result, those tests would fail in a system where `tor` is running as a
daemon.

Closes ooni/probe#2623

---------

Co-authored-by: Ain Ghazal <[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

Successfully merging this pull request may close these issues.

libtor: prevent concurrent instances
2 participants