Skip to content

Commit a890075

Browse files
github-actions[bot]allantarginonoahfalk
authored
[release/10.0] Fix race condition when createdump tries to read /proc/pid/mem before prctl(PR_SET_PTRACER, childpid) is set (#120071)
* Fix race condition in PROCCreateCrashDump when createdump tries to read /proc/pid/mem before prctl is set * addressing PR comments for better naming clarity and write/error checks * Update src/coreclr/pal/src/thread/process.cpp Co-authored-by: Noah Falk <[email protected]> * Update src/coreclr/pal/src/thread/process.cpp Co-authored-by: Noah Falk <[email protected]> * guarding against EINTR * Wait for prctl(PR_SET_PTRACER, childpid) in nativeaot --------- Co-authored-by: Allan Targino <[email protected]> Co-authored-by: Noah Falk <[email protected]>
1 parent 27a3d92 commit a890075

File tree

2 files changed

+96
-26
lines changed

2 files changed

+96
-26
lines changed

‎src/coreclr/nativeaot/Runtime/unix/PalCreateDump.cpp‎

Lines changed: 47 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -213,18 +213,21 @@ CreateCrashDump(
213213
char* errorMessageBuffer,
214214
int cbErrorMessageBuffer)
215215
{
216-
int pipe_descs[2];
217-
if (pipe(pipe_descs) == -1)
216+
int pipe_descs[4];
217+
if (pipe(pipe_descs) == -1 || pipe(pipe_descs + 2) == -1)
218218
{
219219
if (errorMessageBuffer != nullptr)
220220
{
221221
snprintf(errorMessageBuffer, cbErrorMessageBuffer, "Problem launching createdump: pipe() FAILED %s (%d)\n", strerror(errno), errno);
222222
}
223223
return false;
224224
}
225-
// [0] is read end, [1] is write end
226-
int parent_pipe = pipe_descs[0];
227-
int child_pipe = pipe_descs[1];
225+
// from parent (write) to child (read), used to signal prctl(PR_SET_PTRACER, childpid) is done
226+
int child_read_pipe = pipe_descs[0];
227+
int parent_write_pipe = pipe_descs[1];
228+
// from child (write) to parent (read), used to capture createdump's stderr
229+
int parent_read_pipe = pipe_descs[2];
230+
int child_write_pipe = pipe_descs[3];
228231

229232
// Fork the core dump child process.
230233
pid_t childpid = fork();
@@ -236,19 +239,34 @@ CreateCrashDump(
236239
{
237240
snprintf(errorMessageBuffer, cbErrorMessageBuffer, "Problem launching createdump: fork() FAILED %s (%d)\n", strerror(errno), errno);
238241
}
239-
close(pipe_descs[0]);
240-
close(pipe_descs[1]);
242+
for (int i = 0; i < 4; i++)
243+
{
244+
close(pipe_descs[i]);
245+
}
241246
return false;
242247
}
243248
else if (childpid == 0)
244249
{
245-
// Close the read end of the pipe, the child doesn't need it
246-
close(parent_pipe);
250+
close(parent_read_pipe);
251+
close(parent_write_pipe);
252+
253+
// Wait for prctl(PR_SET_PTRACER, childpid) in parent
254+
char buffer;
255+
int bytesRead;
256+
while((bytesRead = read(child_read_pipe, &buffer, 1)) < 0 && errno == EINTR);
257+
close(child_read_pipe);
258+
259+
if (bytesRead != 1)
260+
{
261+
fprintf(stderr, "Problem reading from createdump child_read_pipe: %s (%d)\n", strerror(errno), errno);
262+
close(child_write_pipe);
263+
exit(-1);
264+
}
247265

248266
// Only dup the child's stderr if there is error buffer
249267
if (errorMessageBuffer != nullptr)
250268
{
251-
dup2(child_pipe, STDERR_FILENO);
269+
dup2(child_write_pipe, STDERR_FILENO);
252270
}
253271
// Execute the createdump program
254272
if (execv(argv[0], (char* const *)argv) == -1)
@@ -259,6 +277,8 @@ CreateCrashDump(
259277
}
260278
else
261279
{
280+
close(child_read_pipe);
281+
close(child_write_pipe);
262282
#if HAVE_PRCTL_H && HAVE_PR_SET_PTRACER
263283
// Gives the child process permission to use /proc/<pid>/mem and ptrace
264284
if (prctl(PR_SET_PTRACER, childpid, 0, 0, 0) == -1)
@@ -270,15 +290,29 @@ CreateCrashDump(
270290
#endif
271291
}
272292
#endif // HAVE_PRCTL_H && HAVE_PR_SET_PTRACER
273-
close(child_pipe);
293+
// Signal child that prctl(PR_SET_PTRACER, childpid) is done
294+
int bytesWritten;
295+
while((bytesWritten = write(parent_write_pipe, "S", 1)) < 0 && errno == EINTR);
296+
close(parent_write_pipe);
297+
298+
if (bytesWritten != 1)
299+
{
300+
fprintf(stderr, "Problem writing to createdump parent_write_pipe: %s (%d)\n", strerror(errno), errno);
301+
close(parent_read_pipe);
302+
if (errorMessageBuffer != nullptr)
303+
{
304+
errorMessageBuffer[0] = 0;
305+
}
306+
return false;
307+
}
274308

275309
// Read createdump's stderr messages (if any)
276310
if (errorMessageBuffer != nullptr)
277311
{
278312
// Read createdump's stderr
279313
int bytesRead = 0;
280314
int count = 0;
281-
while ((count = read(parent_pipe, errorMessageBuffer + bytesRead, cbErrorMessageBuffer - bytesRead)) > 0)
315+
while ((count = read(parent_read_pipe, errorMessageBuffer + bytesRead, cbErrorMessageBuffer - bytesRead)) > 0)
282316
{
283317
bytesRead += count;
284318
}
@@ -288,7 +322,7 @@ CreateCrashDump(
288322
fputs(errorMessageBuffer, stderr);
289323
}
290324
}
291-
close(parent_pipe);
325+
close(parent_read_pipe);
292326

293327
// Parent waits until the child process is done
294328
int wstatus = 0;

‎src/coreclr/pal/src/thread/process.cpp‎

Lines changed: 49 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2447,18 +2447,22 @@ PROCCreateCrashDump(
24472447
}
24482448
}
24492449

2450-
int pipe_descs[2];
2451-
if (pipe(pipe_descs) == -1)
2450+
int pipe_descs[4];
2451+
if (pipe(pipe_descs) == -1 || pipe(pipe_descs + 2) == -1)
24522452
{
24532453
if (errorMessageBuffer != nullptr)
24542454
{
24552455
sprintf_s(errorMessageBuffer, cbErrorMessageBuffer, "Problem launching createdump: pipe() FAILED %s (%d)\n", strerror(errno), errno);
24562456
}
24572457
return false;
24582458
}
2459-
// [0] is read end, [1] is write end
2460-
int parent_pipe = pipe_descs[0];
2461-
int child_pipe = pipe_descs[1];
2459+
2460+
// from parent (write) to child (read), used to signal prctl(PR_SET_PTRACER, childpid) is done
2461+
int child_read_pipe = pipe_descs[0];
2462+
int parent_write_pipe = pipe_descs[1];
2463+
// from child (write) to parent (read), used to capture createdump's stderr
2464+
int parent_read_pipe = pipe_descs[2];
2465+
int child_write_pipe = pipe_descs[3];
24622466

24632467
// Fork the core dump child process.
24642468
pid_t childpid = fork();
@@ -2470,20 +2474,36 @@ PROCCreateCrashDump(
24702474
{
24712475
sprintf_s(errorMessageBuffer, cbErrorMessageBuffer, "Problem launching createdump: fork() FAILED %s (%d)\n", strerror(errno), errno);
24722476
}
2473-
close(pipe_descs[0]);
2474-
close(pipe_descs[1]);
2477+
for (int i = 0; i < 4; i++)
2478+
{
2479+
close(pipe_descs[i]);
2480+
}
24752481
return false;
24762482
}
24772483
else if (childpid == 0)
24782484
{
2479-
// Close the read end of the pipe, the child doesn't need it
24802485
int callbackResult = 0;
2481-
close(parent_pipe);
2486+
2487+
close(parent_read_pipe);
2488+
close(parent_write_pipe);
2489+
2490+
// Wait for prctl(PR_SET_PTRACER, childpid) in parent
2491+
char buffer;
2492+
int bytesRead;
2493+
while((bytesRead = read(child_read_pipe, &buffer, 1)) < 0 && errno == EINTR);
2494+
close(child_read_pipe);
2495+
2496+
if (bytesRead != 1)
2497+
{
2498+
fprintf(stderr, "Problem reading from createdump child_read_pipe: %s (%d)\n", strerror(errno), errno);
2499+
close(child_write_pipe);
2500+
exit(-1);
2501+
}
24822502

24832503
// Only dup the child's stderr if there is error buffer
24842504
if (errorMessageBuffer != nullptr)
24852505
{
2486-
dup2(child_pipe, STDERR_FILENO);
2506+
dup2(child_write_pipe, STDERR_FILENO);
24872507
}
24882508
if (g_createdumpCallback != nullptr)
24892509
{
@@ -2510,6 +2530,8 @@ PROCCreateCrashDump(
25102530
}
25112531
else
25122532
{
2533+
close(child_read_pipe);
2534+
close(child_write_pipe);
25132535
#if HAVE_PRCTL_H && HAVE_PR_SET_PTRACER
25142536
// Gives the child process permission to use /proc/<pid>/mem and ptrace
25152537
if (prctl(PR_SET_PTRACER, childpid, 0, 0, 0) == -1)
@@ -2519,15 +2541,29 @@ PROCCreateCrashDump(
25192541
ERROR("PROCCreateCrashDump: prctl() FAILED %s (%d)\n", strerror(errno), errno);
25202542
}
25212543
#endif // HAVE_PRCTL_H && HAVE_PR_SET_PTRACER
2522-
close(child_pipe);
2544+
// Signal child that prctl(PR_SET_PTRACER, childpid) is done
2545+
int bytesWritten;
2546+
while((bytesWritten = write(parent_write_pipe, "S", 1)) < 0 && errno == EINTR);
2547+
close(parent_write_pipe);
2548+
2549+
if (bytesWritten != 1)
2550+
{
2551+
fprintf(stderr, "Problem writing to createdump parent_write_pipe: %s (%d)\n", strerror(errno), errno);
2552+
close(parent_read_pipe);
2553+
if (errorMessageBuffer != nullptr)
2554+
{
2555+
errorMessageBuffer[0] = 0;
2556+
}
2557+
return false;
2558+
}
25232559

25242560
// Read createdump's stderr messages (if any)
25252561
if (errorMessageBuffer != nullptr)
25262562
{
25272563
// Read createdump's stderr
25282564
int bytesRead = 0;
25292565
int count = 0;
2530-
while ((count = read(parent_pipe, errorMessageBuffer + bytesRead, cbErrorMessageBuffer - bytesRead)) > 0)
2566+
while ((count = read(parent_read_pipe, errorMessageBuffer + bytesRead, cbErrorMessageBuffer - bytesRead)) > 0)
25312567
{
25322568
bytesRead += count;
25332569
}
@@ -2537,7 +2573,7 @@ PROCCreateCrashDump(
25372573
fputs(errorMessageBuffer, stderr);
25382574
}
25392575
}
2540-
close(parent_pipe);
2576+
close(parent_read_pipe);
25412577

25422578
// Parent waits until the child process is done
25432579
int wstatus = 0;

0 commit comments

Comments
 (0)