Skip to content

Commit c70d73c

Browse files
committed
Fix a use-after-free with task tracing.
We cannot access the executed job after it has finished executing: - If it's a non-task job, it is always invalidated; such jobs are self-owning, and they are expected to destroy themselves after execution. - If it's a task, and it completes during execution, it will invalidate itself synchronously, e.g. by releasing itself. At this point, it must be assumed that the task memory has been releaed. - If it's a task, and it hasn't completed during execution, we are now racing with whatever event *does* complete the task. Any information we want to log about the job must be recorded when it starts to run. rdar://88817560
1 parent 313165d commit c70d73c

File tree

4 files changed

+35
-20
lines changed

4 files changed

+35
-20
lines changed

stdlib/public/Concurrency/Actor.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1540,7 +1540,7 @@ static void swift_job_runImpl(Job *job, ExecutorRef executor) {
15401540
SWIFT_TASK_DEBUG_LOG("%s(%p)", __func__, job);
15411541
runJobInEstablishedExecutorContext(job);
15421542

1543-
concurrency::trace::job_run_end(job, &executor, traceHandle);
1543+
concurrency::trace::job_run_end(&executor, traceHandle);
15441544
trackingInfo.leave();
15451545

15461546
// Give up the current executor if this is a switching context

stdlib/public/Concurrency/Tracing.h

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,22 @@ void job_enqueue_global_with_delay(unsigned long long delay, Job *job);
7575

7676
void job_enqueue_main_executor(Job *job);
7777

78-
// This returns a handle that must be passed to the corresponding call to
79-
// task_run_end.
80-
uint64_t job_run_begin(Job *job, ExecutorRef *executor);
81-
82-
void job_run_end(Job *job, ExecutorRef *executor, uint64_t beginHandle);
78+
struct job_run_info {
79+
/// The ID of the task that started running.
80+
uint64_t taskId;
81+
82+
/// The signpost ID for this task execution, or OS_SIGNPOST_ID_INVALID
83+
/// if the job was not a task.
84+
uint64_t handle;
85+
};
86+
87+
// This returns information that must be passed to the corresponding
88+
// call to task_run_end. Any information we want to log must be
89+
// extracted from the job when we start to run it because execution
90+
// will invalidate the job.
91+
job_run_info job_run_begin(Job *job, ExecutorRef *executor);
92+
93+
void job_run_end(ExecutorRef *executor, job_run_info info);
8394

8495
} // namespace trace
8596
} // namespace concurrency

stdlib/public/Concurrency/TracingSignpost.h

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -244,29 +244,33 @@ inline void job_enqueue_main_executor(Job *job) {
244244
}
245245
}
246246

247-
inline uint64_t job_run_begin(Job *job, ExecutorRef *executor) {
247+
inline job_run_info job_run_begin(Job *job, ExecutorRef *executor) {
248+
auto invalidInfo = []{
249+
return job_run_info{ 0, OS_SIGNPOST_ID_INVALID };
250+
};
251+
248252
if (AsyncTask *task = dyn_cast<AsyncTask>(job)) {
249-
ENSURE_LOGS(0);
250-
auto id = os_signpost_id_generate(TaskLog);
253+
ENSURE_LOGS(invalidInfo());
254+
auto handle = os_signpost_id_generate(TaskLog);
255+
auto taskId = task->getTaskId();
251256
os_signpost_interval_begin(
252-
TaskLog, id, SWIFT_LOG_JOB_RUN_NAME,
257+
TaskLog, handle, SWIFT_LOG_JOB_RUN_NAME,
253258
"task=%" PRIx64
254259
" executorIdentity=%p executorImplementation=0x%" PRIxPTR,
255-
task->getTaskId(), executor->getIdentity(),
256-
executor->getRawImplementation());
257-
return id;
260+
taskId, executor->getIdentity(), executor->getRawImplementation());
261+
return { taskId, handle };
258262
}
259-
return 0;
263+
return invalidInfo();
260264
}
261265

262-
inline void job_run_end(Job *job, ExecutorRef *executor, uint64_t beginHandle) {
263-
if (AsyncTask *task = dyn_cast<AsyncTask>(job)) {
266+
inline void job_run_end(ExecutorRef *executor, job_run_info info) {
267+
if (info.handle != OS_SIGNPOST_ID_INVALID) {
264268
ENSURE_LOGS();
265269
os_signpost_interval_end(
266-
TaskLog, beginHandle, SWIFT_LOG_JOB_RUN_NAME,
270+
TaskLog, info.handle, SWIFT_LOG_JOB_RUN_NAME,
267271
"task=%" PRIx64
268272
" executorIdentity=%p executorImplementation=0x%" PRIxPTR,
269-
task->getTaskId(), executor->getIdentity(),
273+
info.taskId, executor->getIdentity(),
270274
executor->getRawImplementation());
271275
}
272276
}

stdlib/public/Concurrency/TracingStubs.h

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,9 +60,9 @@ inline void job_enqueue_global_with_delay(unsigned long long delay, Job *job) {}
6060

6161
inline void job_enqueue_main_executor(Job *job) {}
6262

63-
inline uint64_t job_run_begin(Job *job, ExecutorRef *executor) { return 0; }
63+
inline job_run_info job_run_begin(Job *job, ExecutorRef *executor) { return {}; }
6464

65-
inline void job_run_end(Job *job, ExecutorRef *executor, uint64_t beginHandle) {
65+
inline void job_run_end(ExecutorRef *executor, job_run_info info) {
6666
}
6767

6868
} // namespace trace

0 commit comments

Comments
 (0)