Skip to content

Commit 66bcc1e

Browse files
mralephCommit Queue
authored andcommitted
[dartdev] Fix race in DartDev::RunDartDev
It was checking DartDev::result_ was set to determine if dartdev is done, however the intent of the code is to only continue once we receive DartDev_Result_Exit from dartdev (based on the fact that we only Notify in ExitResultCallback). This code was incorrect if we encounter a spurios wakeup: we might reach RunExecResultCallback but only execute it partially i.e. set result_ to DartDev_Result_RunExec but only partially initialize argv_ and argc_, and not yet initialize script_name_. However at this point we might spuriously wakeup from our Wait in RunDartDev and think that we can continue - which means we will hit a crash once we try to use partially initialized information for Process::Exec which would cause us to crash. The bug was discovered based on stack traces provided by Elliott Brooks (elliottbrooks) which occur on Dev Tools CI. TEST=too hard to reliably test this Change-Id: Ied9cace6bec9505b51ac0142795708466d166f28 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/445180 Reviewed-by: Daco Harkes <[email protected]> Commit-Queue: Slava Egorov <[email protected]>
1 parent 67d39ca commit 66bcc1e

File tree

1 file changed

+4
-1
lines changed

1 file changed

+4
-1
lines changed

runtime/bin/dartdev.cc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -712,6 +712,7 @@ class DartDev {
712712
// the result from execution of dartdev.
713713
{
714714
MonitorLocker locker(monitor_);
715+
exited_ = true;
715716
locker.Notify();
716717
}
717718
}
@@ -779,7 +780,7 @@ class DartDev {
779780
// proceed.
780781
{
781782
MonitorLocker locker(monitor_);
782-
while (result_ == DartDev_Result_Unknown) {
783+
while (!exited_) {
783784
locker.Wait();
784785
}
785786
}
@@ -940,6 +941,7 @@ class DartDev {
940941
}
941942

942943
static Monitor* monitor_;
944+
static bool exited_;
943945
static DartDev_Result result_;
944946
static char* script_name_;
945947
static char* package_config_override_;
@@ -949,6 +951,7 @@ class DartDev {
949951
};
950952

951953
Monitor* DartDev::monitor_ = new Monitor();
954+
bool DartDev::exited_ = false;
952955
DartDev::DartDev_Result DartDev::result_ = DartDev::DartDev_Result_Unknown;
953956
char* DartDev::script_name_ = nullptr;
954957
char* DartDev::package_config_override_ = nullptr;

0 commit comments

Comments
 (0)