From 5251ea203d61e15508e4fba64220ca3496f750a3 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 7 Feb 2023 12:56:37 +0100 Subject: [PATCH 01/12] chore: add patch useful for debugging --- CDEPS/tor/004.patch | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 CDEPS/tor/004.patch diff --git a/CDEPS/tor/004.patch b/CDEPS/tor/004.patch new file mode 100644 index 0000000000..4bbe240ee6 --- /dev/null +++ b/CDEPS/tor/004.patch @@ -0,0 +1,30 @@ +diff --git a/src/app/main/main.c b/src/app/main/main.c +index 838e129d04..3aa1b334fa 100644 +--- a/src/app/main/main.c ++++ b/src/app/main/main.c +@@ -92,6 +92,8 @@ + #include + #endif + ++#include ++ + #ifdef HAVE_SYSTEMD + # if defined(__COVERITY__) && !defined(__INCLUDE_LEVEL__) + /* Systemd's use of gcc's __INCLUDE_LEVEL__ extension macro appears to confuse +@@ -1284,8 +1286,16 @@ pubsub_install(void) + { + pubsub_builder_t *builder = pubsub_builder_new(); + int r = subsystems_add_pubsub(builder); ++ if (r != 0) { ++ fprintf(stderr, "SBSDEBUG: subsystems_add_pubsub failed: %d\n", r); ++ /* FALLTHROUGH */ ++ } + tor_assert(r == 0); + r = tor_mainloop_connect_pubsub(builder); // consumes builder ++ if (r != 0) { ++ fprintf(stderr, "SBSDEBUG: tor_mainloop_connect_pubsub failed: %d\n", r); ++ /* FALLTHROUGH */ ++ } + tor_assert(r == 0); + } + From e4fe578899e89382297f02c3f63fd46773d9750b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 7 Feb 2023 16:48:07 +0100 Subject: [PATCH 02/12] chore: continue debugging the crash --- CDEPS/tor/004.patch | 66 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+) diff --git a/CDEPS/tor/004.patch b/CDEPS/tor/004.patch index 4bbe240ee6..530b8c51a0 100644 --- a/CDEPS/tor/004.patch +++ b/CDEPS/tor/004.patch @@ -28,3 +28,69 @@ index 838e129d04..3aa1b334fa 100644 tor_assert(r == 0); } +diff --git a/src/core/mainloop/mainloop_pubsub.c b/src/core/mainloop/mainloop_pubsub.c +index 1e72ada5f0..72dd348a2e 100644 +--- a/src/core/mainloop/mainloop_pubsub.c ++++ b/src/core/mainloop/mainloop_pubsub.c +@@ -26,6 +26,8 @@ + #include "lib/pubsub/pubsub.h" + #include "lib/pubsub/pubsub_build.h" + ++#include ++ + /** + * Dispatcher to use for delivering messages. + **/ +@@ -63,8 +65,10 @@ tor_mainloop_connect_pubsub(struct pubsub_builder_t *builder) + tor_mainloop_disconnect_pubsub(); + + the_dispatcher = pubsub_builder_finalize(builder, &the_pubsub_items); +- if (! the_dispatcher) ++ if (! the_dispatcher) { ++ fprintf(stderr, "SBSDEBUG: the_distpatcher is NULL\n"); + goto err; ++ } + + rv = 0; + goto done; +diff --git a/src/lib/pubsub/pubsub_build.c b/src/lib/pubsub/pubsub_build.c +index 30b9194062..6a554c06bd 100644 +--- a/src/lib/pubsub/pubsub_build.c ++++ b/src/lib/pubsub/pubsub_build.c +@@ -25,7 +25,8 @@ + #include "lib/log/util_bug.h" + #include "lib/malloc/malloc.h" + +- #include ++#include ++#include + + /** Construct and return a new empty pubsub_items_t. */ + static pubsub_items_t * +@@ -281,19 +282,24 @@ pubsub_builder_finalize(pubsub_builder_t *builder, + dispatch_t *dispatcher = NULL; + tor_assert_nonfatal(builder->n_connectors == 0); + +- if (pubsub_builder_check(builder) < 0) ++ if (pubsub_builder_check(builder) < 0) { ++ fprintf(stderr, "SBSDEBUG: pubsub_builder_check failed\n"); + goto err; ++ } + + if (builder->n_errors) { + log_warn(LD_GENERAL, "At least one error occurred previously when " + "configuring the dispatcher."); ++ fprintf(stderr, "SBSDEBUG: builder->n_errors is not zero\n"); + goto err; + } + + dispatcher = dispatch_new(builder->cfg); + +- if (!dispatcher) ++ if (!dispatcher) { ++ fprintf(stderr, "SBSDEBUG: dispatcher_new failed\n"); + goto err; ++ } + + pubsub_items_install_bindings(builder->items, dispatcher); + if (items_out) { From e81cec576a7031b408142f5dc6d365fd04305c62 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 7 Feb 2023 18:03:55 +0100 Subject: [PATCH 03/12] chore: continue debugging the issue --- CDEPS/tor/004.patch | 68 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/CDEPS/tor/004.patch b/CDEPS/tor/004.patch index 530b8c51a0..355e6db946 100644 --- a/CDEPS/tor/004.patch +++ b/CDEPS/tor/004.patch @@ -94,3 +94,71 @@ index 30b9194062..6a554c06bd 100644 pubsub_items_install_bindings(builder->items, dispatcher); if (items_out) { +diff --git a/src/lib/pubsub/pubsub_check.c b/src/lib/pubsub/pubsub_check.c +index 99e604d715..a9ae957551 100644 +--- a/src/lib/pubsub/pubsub_check.c ++++ b/src/lib/pubsub/pubsub_check.c +@@ -25,6 +25,7 @@ + #include "lib/malloc/malloc.h" + #include "lib/string/compat_string.h" + ++#include + #include + + static void pubsub_adjmap_add(pubsub_adjmap_t *map, +@@ -343,21 +344,27 @@ lint_message(const pubsub_adjmap_t *map, message_id_t msg) + log_warn(LD_MESG|LD_BUG, + "Message \"%s\" has subscribers, but no publishers.", + get_message_id_name(msg)); ++ fprintf(stderr, "SBSDEBUG: n_pub == 0\n"); + ok = false; + } else if (n_sub == 0) { + log_warn(LD_MESG|LD_BUG, + "Message \"%s\" has publishers, but no subscribers.", + get_message_id_name(msg)); ++ fprintf(stderr, "SBSDEBUG: n_sub == 0\n"); + ok = false; + } + + /* Check the message graph topology. */ +- if (lint_message_graph(map, msg, pub, sub) < 0) ++ if (lint_message_graph(map, msg, pub, sub) < 0) { ++ fprintf(stderr, "SBSDEBUG: lint_message_graph failed\n"); + ok = false; ++ } + + /* Check whether the messages have the same fields set on them. */ +- if (lint_message_consistency(msg, pub, sub) < 0) ++ if (lint_message_consistency(msg, pub, sub) < 0) { ++ fprintf(stderr, "SBSDEBUG: lint_message_consistency failed\n"); + ok = false; ++ } + + if (!ok) { + /* There was a problem -- let's log all the publishers and subscribers on +@@ -385,6 +392,7 @@ pubsub_adjmap_check(const pubsub_adjmap_t *map) + bool all_ok = true; + for (unsigned i = 0; i < map->n_msgs; ++i) { + if (lint_message(map, i) < 0) { ++ fprintf(stderr, "lint_message failed for %u\n", i); + all_ok = false; + } + } +@@ -401,11 +409,15 @@ pubsub_builder_check(pubsub_builder_t *builder) + pubsub_adjmap_t *map = pubsub_build_adjacency_map(builder->items); + int rv = -1; + +- if (!map) ++ if (!map) { ++ fprintf(stderr, "SBSDEBUG: pubsub_build_adjacency_map failed\n"); + goto err; // should be impossible ++ } + +- if (pubsub_adjmap_check(map) < 0) ++ if (pubsub_adjmap_check(map) < 0) { ++ fprintf(stderr, "SBSDEBUG: pubsub_adjmap_check failed\n"); + goto err; ++ } + + rv = 0; + err: From 2bf3b761c33464ad77646ca7cad9f6f2fbaa828a Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Thu, 9 Feb 2023 07:51:24 +0100 Subject: [PATCH 04/12] fix: simplify patch I could not reproduce anymore; heisenbug? --- CDEPS/tor/004.patch | 96 --------------------------------------------- 1 file changed, 96 deletions(-) diff --git a/CDEPS/tor/004.patch b/CDEPS/tor/004.patch index 355e6db946..ca7fd772a5 100644 --- a/CDEPS/tor/004.patch +++ b/CDEPS/tor/004.patch @@ -1,99 +1,3 @@ -diff --git a/src/app/main/main.c b/src/app/main/main.c -index 838e129d04..3aa1b334fa 100644 ---- a/src/app/main/main.c -+++ b/src/app/main/main.c -@@ -92,6 +92,8 @@ - #include - #endif - -+#include -+ - #ifdef HAVE_SYSTEMD - # if defined(__COVERITY__) && !defined(__INCLUDE_LEVEL__) - /* Systemd's use of gcc's __INCLUDE_LEVEL__ extension macro appears to confuse -@@ -1284,8 +1286,16 @@ pubsub_install(void) - { - pubsub_builder_t *builder = pubsub_builder_new(); - int r = subsystems_add_pubsub(builder); -+ if (r != 0) { -+ fprintf(stderr, "SBSDEBUG: subsystems_add_pubsub failed: %d\n", r); -+ /* FALLTHROUGH */ -+ } - tor_assert(r == 0); - r = tor_mainloop_connect_pubsub(builder); // consumes builder -+ if (r != 0) { -+ fprintf(stderr, "SBSDEBUG: tor_mainloop_connect_pubsub failed: %d\n", r); -+ /* FALLTHROUGH */ -+ } - tor_assert(r == 0); - } - -diff --git a/src/core/mainloop/mainloop_pubsub.c b/src/core/mainloop/mainloop_pubsub.c -index 1e72ada5f0..72dd348a2e 100644 ---- a/src/core/mainloop/mainloop_pubsub.c -+++ b/src/core/mainloop/mainloop_pubsub.c -@@ -26,6 +26,8 @@ - #include "lib/pubsub/pubsub.h" - #include "lib/pubsub/pubsub_build.h" - -+#include -+ - /** - * Dispatcher to use for delivering messages. - **/ -@@ -63,8 +65,10 @@ tor_mainloop_connect_pubsub(struct pubsub_builder_t *builder) - tor_mainloop_disconnect_pubsub(); - - the_dispatcher = pubsub_builder_finalize(builder, &the_pubsub_items); -- if (! the_dispatcher) -+ if (! the_dispatcher) { -+ fprintf(stderr, "SBSDEBUG: the_distpatcher is NULL\n"); - goto err; -+ } - - rv = 0; - goto done; -diff --git a/src/lib/pubsub/pubsub_build.c b/src/lib/pubsub/pubsub_build.c -index 30b9194062..6a554c06bd 100644 ---- a/src/lib/pubsub/pubsub_build.c -+++ b/src/lib/pubsub/pubsub_build.c -@@ -25,7 +25,8 @@ - #include "lib/log/util_bug.h" - #include "lib/malloc/malloc.h" - -- #include -+#include -+#include - - /** Construct and return a new empty pubsub_items_t. */ - static pubsub_items_t * -@@ -281,19 +282,24 @@ pubsub_builder_finalize(pubsub_builder_t *builder, - dispatch_t *dispatcher = NULL; - tor_assert_nonfatal(builder->n_connectors == 0); - -- if (pubsub_builder_check(builder) < 0) -+ if (pubsub_builder_check(builder) < 0) { -+ fprintf(stderr, "SBSDEBUG: pubsub_builder_check failed\n"); - goto err; -+ } - - if (builder->n_errors) { - log_warn(LD_GENERAL, "At least one error occurred previously when " - "configuring the dispatcher."); -+ fprintf(stderr, "SBSDEBUG: builder->n_errors is not zero\n"); - goto err; - } - - dispatcher = dispatch_new(builder->cfg); - -- if (!dispatcher) -+ if (!dispatcher) { -+ fprintf(stderr, "SBSDEBUG: dispatcher_new failed\n"); - goto err; -+ } - - pubsub_items_install_bindings(builder->items, dispatcher); - if (items_out) { diff --git a/src/lib/pubsub/pubsub_check.c b/src/lib/pubsub/pubsub_check.c index 99e604d715..a9ae957551 100644 --- a/src/lib/pubsub/pubsub_check.c From a372027d3a2060fce9d77cbee5c91dd4b2a0f705 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 21 Mar 2023 15:30:08 +0000 Subject: [PATCH 05/12] feat: more changes to facilitate debugging --- internal/cmd/testtorsf/main.go | 69 ++++++++++++++++++++++++++++++++++ internal/tunnel/torembed.go | 2 +- 2 files changed, 70 insertions(+), 1 deletion(-) create mode 100644 internal/cmd/testtorsf/main.go diff --git a/internal/cmd/testtorsf/main.go b/internal/cmd/testtorsf/main.go new file mode 100644 index 0000000000..60ef4fca9e --- /dev/null +++ b/internal/cmd/testtorsf/main.go @@ -0,0 +1,69 @@ +//go:build ooni_libtor + +package main + +import ( + "context" + "net/http" + "time" + + "github.com/apex/log" + "github.com/ooni/probe-cli/v3/internal/experiment/torsf" + "github.com/ooni/probe-cli/v3/internal/kvstore" + "github.com/ooni/probe-cli/v3/internal/model" + "github.com/ooni/probe-cli/v3/internal/model/mocks" + "github.com/ooni/probe-cli/v3/internal/runtimex" +) + +func runit() { + measurer := torsf.NewExperimentMeasurer(torsf.Config{ + DisablePersistentDatadir: false, + DisableProgress: false, + RendezvousMethod: "", + }) + meas := &model.Measurement{} + err := measurer.Run( + context.Background(), + &model.ExperimentArgs{ + Callbacks: model.NewPrinterCallbacks(model.DiscardLogger), + Measurement: meas, + Session: &mocks.Session{ + MockDefaultHTTPClient: func() model.HTTPClient { + return http.DefaultClient + }, + MockKeyValueStore: func() model.KeyValueStore { + return &kvstore.Memory{} + }, + MockLogger: func() model.Logger { + return log.Log + }, + MockSoftwareName: func() string { + return "miniooni" + }, + MockSoftwareVersion: func() string { + return "0.1.0-dev" + }, + MockTempDir: func() string { + return "x/tmp" + }, + MockTunnelDir: func() string { + return "x/tunnel" + }, + MockUserAgent: func() string { + return model.HTTPHeaderUserAgent + }, + }, + }, + ) + runtimex.PanicOnError(err, "measurer.Run failed") + tk := meas.TestKeys.(*torsf.TestKeys) + runtimex.Assert(tk.Success, "did not succeed") +} + +func main() { + for { + runit() + log.Info("************* now let's wait a bit ********************************") + time.Sleep(45 * time.Second) + } +} diff --git a/internal/tunnel/torembed.go b/internal/tunnel/torembed.go index df7bf67991..e6dec63f9c 100644 --- a/internal/tunnel/torembed.go +++ b/internal/tunnel/torembed.go @@ -1,4 +1,4 @@ -//go:build ooni_libtor && android +//go:build ooni_libtor && (android || linux) package tunnel From ea2a628de7a811e31bbdd799c2da566d09c3e771 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Tue, 21 Mar 2023 17:31:22 +0100 Subject: [PATCH 06/12] fix: make patch log more interesting info --- CDEPS/tor/004.patch | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/CDEPS/tor/004.patch b/CDEPS/tor/004.patch index ca7fd772a5..8b0556720a 100644 --- a/CDEPS/tor/004.patch +++ b/CDEPS/tor/004.patch @@ -1,5 +1,5 @@ diff --git a/src/lib/pubsub/pubsub_check.c b/src/lib/pubsub/pubsub_check.c -index 99e604d715..a9ae957551 100644 +index 99e604d715..a5cc4b7658 100644 --- a/src/lib/pubsub/pubsub_check.c +++ b/src/lib/pubsub/pubsub_check.c @@ -25,6 +25,7 @@ @@ -14,27 +14,27 @@ index 99e604d715..a9ae957551 100644 log_warn(LD_MESG|LD_BUG, "Message \"%s\" has subscribers, but no publishers.", get_message_id_name(msg)); -+ fprintf(stderr, "SBSDEBUG: n_pub == 0\n"); ++ fprintf(stderr, "SBSDEBUG: n_pub == 0 for %s\n", get_message_id_name(msg)); ok = false; } else if (n_sub == 0) { log_warn(LD_MESG|LD_BUG, "Message \"%s\" has publishers, but no subscribers.", get_message_id_name(msg)); -+ fprintf(stderr, "SBSDEBUG: n_sub == 0\n"); ++ fprintf(stderr, "SBSDEBUG: n_sub == 0 for %s\n", get_message_id_name(msg)); ok = false; } /* Check the message graph topology. */ - if (lint_message_graph(map, msg, pub, sub) < 0) + if (lint_message_graph(map, msg, pub, sub) < 0) { -+ fprintf(stderr, "SBSDEBUG: lint_message_graph failed\n"); ++ fprintf(stderr, "SBSDEBUG: lint_message_graph failed for %s\n", get_message_id_name(msg)); ok = false; + } /* Check whether the messages have the same fields set on them. */ - if (lint_message_consistency(msg, pub, sub) < 0) + if (lint_message_consistency(msg, pub, sub) < 0) { -+ fprintf(stderr, "SBSDEBUG: lint_message_consistency failed\n"); ++ fprintf(stderr, "SBSDEBUG: lint_message_consistency failed for %s\n", get_message_id_name(msg)); ok = false; + } @@ -44,7 +44,7 @@ index 99e604d715..a9ae957551 100644 bool all_ok = true; for (unsigned i = 0; i < map->n_msgs; ++i) { if (lint_message(map, i) < 0) { -+ fprintf(stderr, "lint_message failed for %u\n", i); ++ fprintf(stderr, "SBSDEBUG: lint_message failed for %u %s\n", i, get_message_id_name((message_id_t)i)); all_ok = false; } } From 143373422bfdea467b3768f0f016494aeadc335e Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 22 Mar 2023 08:23:36 +0000 Subject: [PATCH 07/12] try to compile tor using tsan --- internal/cmd/buildtool/linuxcdeps.go | 31 +++++++++++++++------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/internal/cmd/buildtool/linuxcdeps.go b/internal/cmd/buildtool/linuxcdeps.go index e238b738af..5249e9baa7 100644 --- a/internal/cmd/buildtool/linuxcdeps.go +++ b/internal/cmd/buildtool/linuxcdeps.go @@ -44,26 +44,29 @@ func linuxCdepsBuildMain(name string, deps buildtoolmodel.Dependencies) { "-fPIC", // makes more sense than -fPIE given that we're building a library "-fsanitize=bounds", "-fsanitize-undefined-trap-on-error", + "-fsanitize=thread", "-O2", } destdir := runtimex.Try1(filepath.Abs(filepath.Join( // must be absolute "internal", "libtor", "linux", runtime.GOARCH, ))) globalEnv := &cBuildEnv{ - ANDROID_HOME: "", - ANDROID_NDK_ROOT: "", - AR: "", - BINPATH: "", - CC: "", - CFLAGS: cflags, - CONFIGURE_HOST: "", - DESTDIR: destdir, - CXX: "", - CXXFLAGS: cflags, - GOARCH: "", - GOARM: "", - LD: "", - LDFLAGS: []string{}, + ANDROID_HOME: "", + ANDROID_NDK_ROOT: "", + AR: "", + BINPATH: "", + CC: "", + CFLAGS: cflags, + CONFIGURE_HOST: "", + DESTDIR: destdir, + CXX: "", + CXXFLAGS: cflags, + GOARCH: "", + GOARM: "", + LD: "", + LDFLAGS: []string{ + "-fsanitize=thread", + }, OPENSSL_API_DEFINE: "", OPENSSL_COMPILER: "linux-x86_64", RANLIB: "", From fc6d0b6e04175722ce77e40cda613c6b009ec02f Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 22 Mar 2023 08:26:38 +0000 Subject: [PATCH 08/12] Revert "try to compile tor using tsan" This reverts commit 143373422bfdea467b3768f0f016494aeadc335e. --- internal/cmd/buildtool/linuxcdeps.go | 31 +++++++++++++--------------- 1 file changed, 14 insertions(+), 17 deletions(-) diff --git a/internal/cmd/buildtool/linuxcdeps.go b/internal/cmd/buildtool/linuxcdeps.go index 5249e9baa7..e238b738af 100644 --- a/internal/cmd/buildtool/linuxcdeps.go +++ b/internal/cmd/buildtool/linuxcdeps.go @@ -44,29 +44,26 @@ func linuxCdepsBuildMain(name string, deps buildtoolmodel.Dependencies) { "-fPIC", // makes more sense than -fPIE given that we're building a library "-fsanitize=bounds", "-fsanitize-undefined-trap-on-error", - "-fsanitize=thread", "-O2", } destdir := runtimex.Try1(filepath.Abs(filepath.Join( // must be absolute "internal", "libtor", "linux", runtime.GOARCH, ))) globalEnv := &cBuildEnv{ - ANDROID_HOME: "", - ANDROID_NDK_ROOT: "", - AR: "", - BINPATH: "", - CC: "", - CFLAGS: cflags, - CONFIGURE_HOST: "", - DESTDIR: destdir, - CXX: "", - CXXFLAGS: cflags, - GOARCH: "", - GOARM: "", - LD: "", - LDFLAGS: []string{ - "-fsanitize=thread", - }, + ANDROID_HOME: "", + ANDROID_NDK_ROOT: "", + AR: "", + BINPATH: "", + CC: "", + CFLAGS: cflags, + CONFIGURE_HOST: "", + DESTDIR: destdir, + CXX: "", + CXXFLAGS: cflags, + GOARCH: "", + GOARM: "", + LD: "", + LDFLAGS: []string{}, OPENSSL_API_DEFINE: "", OPENSSL_COMPILER: "linux-x86_64", RANLIB: "", From 8a0118d214e1cff2cd1b4577c9bd2ea2c8b7f849 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 22 Mar 2023 08:27:38 +0000 Subject: [PATCH 09/12] can I get the crash without snowflake? --- internal/cmd/testtorsf/main.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/internal/cmd/testtorsf/main.go b/internal/cmd/testtorsf/main.go index 60ef4fca9e..5df408fa41 100644 --- a/internal/cmd/testtorsf/main.go +++ b/internal/cmd/testtorsf/main.go @@ -8,7 +8,7 @@ import ( "time" "github.com/apex/log" - "github.com/ooni/probe-cli/v3/internal/experiment/torsf" + "github.com/ooni/probe-cli/v3/internal/experiment/vanillator" "github.com/ooni/probe-cli/v3/internal/kvstore" "github.com/ooni/probe-cli/v3/internal/model" "github.com/ooni/probe-cli/v3/internal/model/mocks" @@ -16,10 +16,10 @@ import ( ) func runit() { - measurer := torsf.NewExperimentMeasurer(torsf.Config{ - DisablePersistentDatadir: false, - DisableProgress: false, - RendezvousMethod: "", + measurer := vanillator.NewExperimentMeasurer(vanillator.Config{ + //DisablePersistentDatadir: false, + DisableProgress: false, + //RendezvousMethod: "", }) meas := &model.Measurement{} err := measurer.Run( @@ -56,7 +56,7 @@ func runit() { }, ) runtimex.PanicOnError(err, "measurer.Run failed") - tk := meas.TestKeys.(*torsf.TestKeys) + tk := meas.TestKeys.(*vanillator.TestKeys) runtimex.Assert(tk.Success, "did not succeed") } From 51292a49e41ae9283b8212726c466d90ff1d9d38 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 22 Mar 2023 09:49:03 +0000 Subject: [PATCH 10/12] check whether pinning to the OS thread fixes the issue --- internal/libtor/enabled.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/libtor/enabled.go b/internal/libtor/enabled.go index cd20044d0f..f877272e21 100644 --- a/internal/libtor/enabled.go +++ b/internal/libtor/enabled.go @@ -149,6 +149,13 @@ var ErrNonzeroExitCode = errors.New("libtor: command completed with nonzero exit // runtor runs tor until completion and ensures that tor exits when // the given ctx is cancelled or its deadline expires. func (p *torProcess) runtor(ctx context.Context, cc net.Conn, args ...string) { + // make sure we lock to an OS thread otherwise the goroutine can get + // preempted midway and cause data races + // + // See https://github.com/ooni/probe/issues/2406#issuecomment-1479138677 + runtime.LockOSThread() + defer runtime.UnlockOSThread() + // wait for Start or context to expire select { case <-p.awaitStart: From 9e5e96710f9b0f57ee5c8346fece4745375c0560 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 22 Mar 2023 09:49:40 +0000 Subject: [PATCH 11/12] add missing runtime import --- internal/libtor/enabled.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/libtor/enabled.go b/internal/libtor/enabled.go index f877272e21..64daa7ef44 100644 --- a/internal/libtor/enabled.go +++ b/internal/libtor/enabled.go @@ -57,6 +57,7 @@ import ( "fmt" "net" "os" + "runtime" "sync" "github.com/cretz/bine/process" From ef7ec1494b3d5f84233006034a1888d63e726393 Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 22 Mar 2023 13:31:28 +0000 Subject: [PATCH 12/12] feat: bypass Go code and just use C code --- internal/cmd/buildtool/linuxcdeps.go | 2 ++ internal/libtor/enabled.go | 2 +- y/compile.bash | 3 ++ y/main.c | 53 ++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 1 deletion(-) create mode 100755 y/compile.bash create mode 100644 y/main.c diff --git a/internal/cmd/buildtool/linuxcdeps.go b/internal/cmd/buildtool/linuxcdeps.go index e238b738af..1edbf34848 100644 --- a/internal/cmd/buildtool/linuxcdeps.go +++ b/internal/cmd/buildtool/linuxcdeps.go @@ -44,7 +44,9 @@ func linuxCdepsBuildMain(name string, deps buildtoolmodel.Dependencies) { "-fPIC", // makes more sense than -fPIE given that we're building a library "-fsanitize=bounds", "-fsanitize-undefined-trap-on-error", + "-fsanitize=thread", "-O2", + "-g", } destdir := runtimex.Try1(filepath.Abs(filepath.Join( // must be absolute "internal", "libtor", "linux", runtime.GOARCH, diff --git a/internal/libtor/enabled.go b/internal/libtor/enabled.go index 64daa7ef44..dc987e356d 100644 --- a/internal/libtor/enabled.go +++ b/internal/libtor/enabled.go @@ -151,7 +151,7 @@ var ErrNonzeroExitCode = errors.New("libtor: command completed with nonzero exit // the given ctx is cancelled or its deadline expires. func (p *torProcess) runtor(ctx context.Context, cc net.Conn, args ...string) { // make sure we lock to an OS thread otherwise the goroutine can get - // preempted midway and cause data races + // preempted midway and causes data races // // See https://github.com/ooni/probe/issues/2406#issuecomment-1479138677 runtime.LockOSThread() diff --git a/y/compile.bash b/y/compile.bash new file mode 100755 index 0000000000..5d1dc2f90b --- /dev/null +++ b/y/compile.bash @@ -0,0 +1,3 @@ +#!/bin/bash +set -euxo pipefail +gcc -g -Wall -Wextra -fsanitize=thread -I internal/libtor/linux/amd64/include -L internal/libtor/linux/amd64/lib y/main.c -ltor -levent -lssl -lcrypto -lz -lm diff --git a/y/main.c b/y/main.c new file mode 100644 index 0000000000..dd7c1f13dc --- /dev/null +++ b/y/main.c @@ -0,0 +1,53 @@ +#include + +#include +#include +#include +#include + +static void *threadMain(void *ptr) { + int *fdp = (int*)ptr; + (void)sleep(45 /* second */); + (void)close(*fdp); + free(fdp); + return NULL; +} + +int main() { + for (;;) { + tor_main_configuration_t *config = tor_main_configuration_new(); + if (config == NULL) { + exit(1); + } + char *argv[] = { + "tor", + "Log", + "notice stderr", + "DataDirectory", + "./x", + NULL, + }; + int argc = 5; + if (tor_main_configuration_set_command_line(config, argc, argv) != 0) { + exit(2); + } + int filedesc = tor_main_configuration_setup_control_socket(config); + if (filedesc < 0) { + exit(3); + } + int *fdp = malloc(sizeof(*fdp)); + if (fdp == NULL) { + exit(4); + } + *fdp = filedesc; + pthread_t thread; + if (pthread_create(&thread, NULL, threadMain, /* move */ fdp) != 0) { + exit(5); + } + tor_run_main(config); + if (pthread_join(thread, NULL) != 0) { + exit(6); + } + fprintf(stderr, "********** doing another round\n"); + } +}