This is an archive of the discontinued LLVM Phabricator instance.

Differential D113046

[sanitizer] Mark before deref in PosixSpawnImpl
ClosedPublic

Authored by tamird on Nov 2 2021, 1:28 PM.

Details

Reviewers
vitalybuka
eugenis
morehouse
Commits
rG33d9b7b4b26d: [sanitizer] Mark before deref in PosixSpawnImpl
Summary

Read each pointer in the argv and envp arrays before dereferencing it;
this correctly marks an error when these pointers point into memory that
has been freed.

Also accept nullptr argv and envp on Linux. Per the posix_spawn manual:

The argv and envp arguments specify the argument list and environment
for the program that is executed in the child process, as for execve(2).

and then the execve manual:

On Linux, argv and envp can be specified as NULL. In both cases, this
has the same effect as specifying the argument as a pointer to a list
containing a single null pointer.

Diff Detail

Event Timeline

tamird requested review of this revision.Nov 2 2021, 1:28 PM
tamird created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 2 2021, 1:28 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
Harbormaster completed remote builds in B132043: Diff 384204.Nov 2 2021, 1:34 PM
tamird updated this revision to Diff 384209.Nov 2 2021, 1:39 PM
  • Deref after marking pointer as read
  • Allow nullptr argv and envp on Linux per the man page
tamird updated this revision to Diff 384210.Nov 2 2021, 1:42 PM

Pass length rather than end

tamird updated this revision to Diff 384215.Nov 2 2021, 1:46 PM

Read sizeof rather than 1

vitalybuka added inline comments.Nov 2 2021, 2:09 PM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
2435

Hm, somehow I missed it, however I don't see envp == null in man page.
Lets keep them both.
And can you please remove #if, and do that for all platforms.

2438

I don't think order of the check is important. Main goal is to stop invalid call, and it should happens in both cases.

However I don't see why it was off by 1

vitalybuka added inline comments.Nov 2 2021, 2:15 PM
compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
2438

I don't think order of the check is important. Main goal is to stop invalid call, and it should happens in both cases.

Ignore this part, you patch is definitely improvement.

However I don't see why it was off by 1

Still I'd like to see answer to this.

tamird updated this revision to Diff 384232.Nov 2 2021, 2:23 PM

De-special case Linux; amend description

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc
2435

From the posix_spawn man page:

The argv and envp arguments specify the argument list and environment for the program that is executed in the child process, as for execve(2).

From the execve man page:

On Linux, argv and envp can be specified as NULL.

Done (removed #if).

2438

Ah, I think you're right that it wasn't off by 1; I've updated the commit message.

phosek added a subscriber: phosek.Nov 2 2021, 2:27 PM
Harbormaster completed remote builds in B132065: Diff 384232.Nov 2 2021, 2:41 PM
vitalybuka accepted this revision.Nov 2 2021, 3:03 PM
This revision is now accepted and ready to land.Nov 2 2021, 3:03 PM
vitalybuka added a comment.Nov 2 2021, 3:03 PM

Thanks!

tamird added a comment.Nov 2 2021, 3:44 PM

Could someone commit this for me? I don't have access.

tamird updated this revision to Diff 384274.Nov 2 2021, 4:31 PM
tamird retitled this revision from [sanitizer] Fix PosixSpawnImpl off-by-one to [sanitizer] Mark before deref in PosixSpawnImpl.
tamird edited the summary of this revision. (Show Details)

Update message

Harbormaster completed remote builds in B132099: Diff 384274.Nov 2 2021, 4:49 PM
Closed by commit rG33d9b7b4b26d: [sanitizer] Mark before deref in PosixSpawnImpl (authored by tamird, committed by haowei). · Explain WhyNov 3 2021, 10:18 AM
This revision was automatically updated to reflect the committed changes.
haowei added a commit: rG33d9b7b4b26d: [sanitizer] Mark before deref in PosixSpawnImpl.

Revision Contents

PathSize
compiler-rt/
lib/
sanitizer_common/
22 lines

Diff 384504

compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc

  • This file is larger than 256 KB, so syntax highlighting is disabled by default.
Show First 20 Lines • Show All 2,425 Lines • ▼ Show 20 Lines
template <class RealSpawnPtr> template <class RealSpawnPtr>
static int PosixSpawnImpl(void *ctx, RealSpawnPtr *real_posix_spawn, pid_t *pid, static int PosixSpawnImpl(void *ctx, RealSpawnPtr *real_posix_spawn, pid_t *pid,
const char *file_or_path, const void *file_actions, const char *file_or_path, const void *file_actions,
const void *attrp, char *const argv[], const void *attrp, char *const argv[],
char *const envp[]) { char *const envp[]) {
COMMON_INTERCEPTOR_READ_RANGE(ctx, file_or_path, COMMON_INTERCEPTOR_READ_RANGE(ctx, file_or_path,
internal_strlen(file_or_path) + 1); internal_strlen(file_or_path) + 1);
char *const *s = argv; if (argv) {
for (; *s; ++s) for (char *const *s = argv; ; ++s) {
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

Hm, somehow I missed it, however I don't see envp == null in man page.
Lets keep them both.
And can you please remove #if, and do that for all platforms.

vitalybuka: Hm, somehow I missed it, however I don't see envp == null in man page. Lets keep them both. And…
tamirdAuthorUnsubmitted
Done
ReplyInline Actions

From the posix_spawn man page:

The argv and envp arguments specify the argument list and environment for the program that is executed in the child process, as for execve(2).

From the execve man page:

On Linux, argv and envp can be specified as NULL.

Done (removed #if).

tamird: From the [[ https://man7.org/linux/man-pages/man3/posix_spawn.3.html | posix_spawn man page ]]…
COMMON_INTERCEPTOR_READ_RANGE(ctx, s, sizeof(*s));
if (!*s) break;
COMMON_INTERCEPTOR_READ_RANGE(ctx, *s, internal_strlen(*s) + 1); COMMON_INTERCEPTOR_READ_RANGE(ctx, *s, internal_strlen(*s) + 1);
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

I don't think order of the check is important. Main goal is to stop invalid call, and it should happens in both cases.

However I don't see why it was off by 1

vitalybuka: I don't think order of the check is important. Main goal is to stop invalid call, and it should…
vitalybukaUnsubmitted
Not Done
ReplyInline Actions

I don't think order of the check is important. Main goal is to stop invalid call, and it should happens in both cases.

Ignore this part, you patch is definitely improvement.

However I don't see why it was off by 1

Still I'd like to see answer to this.

vitalybuka: > I don't think order of the check is important. Main goal is to stop invalid call, and it…
tamirdAuthorUnsubmitted
Done
ReplyInline Actions

Ah, I think you're right that it wasn't off by 1; I've updated the commit message.

tamird: Ah, I think you're right that it wasn't off by 1; I've updated the commit message.
COMMON_INTERCEPTOR_READ_RANGE(ctx, argv, (s - argv + 1) * sizeof(*s)); }
s = envp; }
for (; *s; ++s) if (envp) {
for (char *const *s = envp; ; ++s) {
COMMON_INTERCEPTOR_READ_RANGE(ctx, s, sizeof(*s));
if (!*s) break;
COMMON_INTERCEPTOR_READ_RANGE(ctx, *s, internal_strlen(*s) + 1); COMMON_INTERCEPTOR_READ_RANGE(ctx, *s, internal_strlen(*s) + 1);
COMMON_INTERCEPTOR_READ_RANGE(ctx, envp, (s - envp + 1) * sizeof(*s)); }
}
int res = int res =
real_posix_spawn(pid, file_or_path, file_actions, attrp, argv, envp); real_posix_spawn(pid, file_or_path, file_actions, attrp, argv, envp);
if (res == 0) if (res == 0)
COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pid, sizeof(*pid)); COMMON_INTERCEPTOR_WRITE_RANGE(ctx, pid, sizeof(*pid));
return res; return res;
} }
INTERCEPTOR(int, posix_spawn, pid_t *pid, const char *path, INTERCEPTOR(int, posix_spawn, pid_t *pid, const char *path,
const void *file_actions, const void *attrp, char *const argv[], const void *file_actions, const void *attrp, char *const argv[],
▲ Show 20 Lines • Show All 8,073 Lines • Show Last 20 Lines