This is an archive of the discontinued LLVM Phabricator instance.

Fix getting environment variables for sanitizers needs on FreeBSD
ClosedPublic

Authored by kutuzov.viktor.84 on Jun 20 2014, 6:33 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kutuzov.viktor.84 retitled this revision from to Fix getting environment variables for sanitizers needs on FreeBSD.
kutuzov.viktor.84 updated this object.
kutuzov.viktor.84 edited the test plan for this revision. (Show Details)
kutuzov.viktor.84 added reviewers: kcc, samsonov.
kutuzov.viktor.84 added subscribers: theraven, emaste, Unknown Object (MLST).
emaste added inline comments.Jun 23 2014, 7:34 AM
lib/sanitizer_common/sanitizer_linux.cc
325 ↗(On Diff #10693)

Note that ::environ may be null - see the libc implementation here:
http://svnweb.freebsd.org/base/head/lib/libc/stdlib/getenv.c?revision=253413&view=markup#l428

samsonov added inline comments.Jun 24 2014, 12:05 AM
lib/sanitizer_common/sanitizer_linux.cc
325 ↗(On Diff #10693)

Why can't you use internal_strstr function here?

lib/sanitizer_common/sanitizer_linux.cc
325 ↗(On Diff #10693)

I believe strstr() doesn't look up through an array of pointers? Or, if you mean to replace internal_strncmp(*Env, name, NameLen) == 0 with internal_strstr(*Env, name) == *Env, well, it would look a bit awkward to me. But no problem to fix, of course.

samsonov added inline comments.Jun 24 2014, 11:26 AM
lib/sanitizer_common/sanitizer_linux.cc
325 ↗(On Diff #10693)

Ah, sorry, please disregard my comment.

kutuzov.viktor.84 added a comment.EditedJul 6 2014, 5:35 AM

Dmitry, your commit rL211417 does the same, but makes things libc-dependent. Do you think we should drop down this patch or replace your implementation? Thanks.

Update: getenv() relies on strncmp() and other intercepted funcs and thus breaks Asan initialization.

I all in favor of removing libc dependencies.
But where does "extern char **environ" came from? Doesn't it come from libc?

emaste added a comment.Jul 7 2014, 4:46 AM

But where does "extern char **environ" came from? Doesn't it come from libc?

char **environ is provided by crt1 - it's here on FreeBSD -HEAD:
http://svnweb.freebsd.org/base/head/lib/csu/common/ignore_init.c?revision=245133&view=markup

I see.
Does the current version work on freebsd?
What's the purpose of removing libc dependency from there?

crt1 is a part of C runtime. I am interested in complete independence of C world for Go runtime.
But I see that now it's dependent on lib anyway, e.g. there is a call to pthread_self.

Does the current version work on freebsd?

Yes, though I need to update the patch to handle the case with null 'environ' first. Will do ASAP.

What's the purpose of removing libc dependency from there?

AFAIU, the less we tied to libc in things like that, the less the need to handle under-initialized sanitizers in intercepted functions.

crt1 is a part of C runtime. I am interested in complete independence of C world for Go runtime.

We would need a special implementation for Go then, I believe.

Updated to reflect recent changes and to test ::environ for NULL.

dvyukov added inline comments.Jul 8 2014, 3:24 AM
lib/sanitizer_common/sanitizer_linux.cc
354 ↗(On Diff #11145)

s/NULL/0/ here in elsewhere
we don't use NULL in sanitizer runtimes, it may not even be defined here

356 ↗(On Diff #11145)

this won't work w/o include <stdlib.h> that you removed
replace it with #error "unsupported platform"

Updated as suggested.

dvyukov accepted this revision.Jul 8 2014, 5:23 AM
dvyukov added a reviewer: dvyukov.
This revision is now accepted and ready to land.Jul 8 2014, 5:23 AM
kutuzov.viktor.84 updated this revision to Diff 11246.

Closed by commit rL212690 (authored by vkutuzov).