Page MenuHomePhabricator

[Sanitizers] Intercept opendir()
ClosedPublic

Authored by kutuzov.viktor.84 on Jan 14 2015, 8:17 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kutuzov.viktor.84 retitled this revision from to [Sanitizers] Intercept opendir().
kutuzov.viktor.84 updated this object.
kutuzov.viktor.84 edited the test plan for this revision. (Show Details)
kutuzov.viktor.84 added subscribers: Unknown Object (MLST), emaste.
kcc edited edge metadata.Jan 14 2015, 6:05 PM

Not sure I understand this.
If opendir() calls strlen(), which we already intersept, won't we see the access w/o this interceptor?

Not sure I understand this.
If opendir() calls strlen(), which we already intersept, won't we see the access w/o this interceptor?

I thought that glibc usually forces references to it's own functions to be linked internally (to avoid PLT costs). So ASan interceptor may not be called.

eugenis edited edge metadata.Jan 16 2015, 1:56 AM

So the difference in observable behavior is we start detecting uninit in the opendir() argument? Why is this freebsd-specific?

Also, there should be a test.

kutuzov.viktor.84 planned changes to this revision.Jan 16 2015, 6:32 AM

If opendir() calls strlen(), which we already intersept, won't we see the access w/o this interceptor?

The point is that FreeBSD's opendir() calls strlen() for inspecting related internal structures and not the file path passed (the comment should mention it; my mistake). Then given we don't intercept opendir(), we indeed do catch poisoned file paths, but we also generate false positives on these internal structures. Since COMMON_INTERCEPTOR_ENTER() includes the declaration for interceptor scope, it suppresses the checks for whatever the real opendir() relies on.

kutuzov.viktor.84 edited edge metadata.
  • The opendir() interceptor moved to Msan so it doesn't interfere with the Tsan's one;
  • The comment fixed to mention internal structures that cause false positives;
  • A test added.

Got it.
This interceptor is beneficial on linux, too - it adds check for opendir() path.
Also, we've been moving interceptors to sanitizer_common, not the other way around. Please move it back, and merge the TSan bits into it (see COMMON_INTERCEPTOR_FD_ACQUIRE).

Looks ok, but I dislike the use of Dir2addr() in common interceptors, which is only valid in TSan and looks completely out of context. Maybe wrap it into COMMON_INTERCEPTOR_ACQUIRE_DIR?
Dmitry?

kutuzov.viktor.84 edited edge metadata.

Updated.

eugenis accepted this revision.Jan 20 2015, 3:51 AM
eugenis edited edge metadata.

Perhaps COMMON_INTERCEPTOR_DIR_ACQUIRE, for similarity with FD_ACQUIRE / FD_RELEASE and MUTEX_LOCK / MUTEX_UNLOCK.

This revision is now accepted and ready to land.Jan 20 2015, 3:51 AM
This revision was automatically updated to reflect the committed changes.