Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make write_debug_image MSAN-safe #5691

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Make write_debug_image MSAN-safe #5691

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24ac3fc
Make write_debug_image MSAN-safe
steven-johnson Feb 1, 2021
a21837f
Merge branch 'master' into srj/msan-dtf
steven-johnson Feb 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions src/runtime/write_debug_image.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,16 +108,19 @@ WEAK bool ends_with(const char *filename, const char *suffix) {
}

struct ScopedFile {
void *f;
ALWAYS_INLINE ScopedFile(const char *filename, const char *mode) {
f = fopen(filename, mode);
void *const user_context;
void *const f;
ALWAYS_INLINE ScopedFile(void *user_context, const char *filename, const char *mode)
: user_context(user_context), f(fopen(filename, mode)) {
halide_msan_annotate_memory_is_initialized(user_context, &f, sizeof(f));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment explaining that args to a syscall that live on the stack must be annotated when msan is on. I guess the filename and mode always come from .rodata? Otherwise this code is baffling.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's the problem. The filename and mode come from code with msan enabled, so they are annotated correctly. The problem here is that fopen wasn't built with msan, so the returned value looks uninitialized to msan.

}
ALWAYS_INLINE ~ScopedFile() {
if (f) {
fclose(f);
}
}
ALWAYS_INLINE bool write(const void *ptr, size_t bytes) {
halide_msan_annotate_memory_is_initialized(user_context, ptr, bytes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this one necessary? We should annotate at the call site of this function when needed, not here. Writing uninitialized memory to a file does seem like it should in fact be an msan failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that the memory isn't uninitialized, but msan's interceptor for fwrite() thinks it is:

  • Calling app allocates a large stack buffer, only uses a small amount of it, so msan adds a 'shadow' record noting that most of this stack area is uninitialized
  • Calling app later calls into Halide and calls this function
  • This function always copies from heap memory into a 4k stack buffer for I/O purposes (so that it can ignore variations in strides)
  • If we get (un)lucky, that stack buffer overlaps with the area of the buffer from step Bounds checking on input images #1; since our runtime isn't compiled with MSAN enabled, the 'shadow' records aren't updated
  • Call into fwrite(), which is intercepted by msan, sees that there is a shadow record for this memory range, sees it has uninitialized bytes, and goes boom

That said... this is a gigantic hack and I probably should rethink this; it assumes/relies on the fact that everything we pass to fwrite() goes through a stack allocation of some sort (which is true) but doesn't actually enforce it (so code changes could easily break this).

Alternate approaches that might make sense:

  • Move all buffering for fwrite() into the ScopedFile class, so we can have ironclad assurance that the memory is something safe to mark properly. (Less hacky code, but would still let us write potentially-random bytes to file)
  • Do msan-specific compiles for all runtime modules for msan, so that our runtime code isn't a special flower. (Downside is that our embedded bitcode ~doubles in size for a rarely-used case, and potentially dramatically slows down runtime code in this case too, though expecting decent performance in an MSAN run is not a thing in the first place)
  • Just declare that this function can't be used in conjunction with MSAN (and add some checks to ensure that the failure is reliable)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a simple solution would be to use __msan_check_mem_is_initialized to check the memory before we buffer it, and then annotate it as initialized after we buffer it?

return f ? fwrite(ptr, bytes, 1, f) > 0 : false;
}
ALWAYS_INLINE bool open() const {
Expand All @@ -144,7 +147,7 @@ WEAK extern "C" int32_t halide_debug_to_file(void *user_context, const char *fil

halide_copy_to_host(user_context, buf);

ScopedFile f(filename, "wb");
ScopedFile f(user_context, filename, "wb");
if (!f.open()) {
return -2;
}
Expand Down