This is an archive of the discontinued LLVM Phabricator instance.

Differential D106631

[dfsan] Handle more uninstrumented functions
Needs ReviewPublic

Authored by gbalats on Jul 22 2021, 8:02 PM.

Details

Reviewers
stephan.yichao.zhao

Diff Detail

Event Timeline

gbalats requested review of this revision.Jul 22 2021, 8:02 PM
gbalats created this revision.
Herald added a subscriber: Restricted Project. · View Herald TranscriptJul 22 2021, 8:02 PM
stephan.yichao.zhao added inline comments.Jul 22 2021, 9:09 PM
compiler-rt/lib/dfsan/done_abilist.txt
76

Is this discard?

https://man7.org/linux/man-pages/man3/isatty.3.html

isatty() returns 1 if fd is an open file descriptor referring to a terminal; otherwise 0 is returned, and errno is set to indicate the error.

This sounds like the return value has no direct dependency on fd.

159

This one probably needs a custom wrapper.

Here is an mac os implementation.
We can see

fp->_file = fd;

So we need to propagate fd's label to fp's _file field. It would be great to find a linux implementation to confirm this.

163

From this uncommon implementation, the return value may depend on the contents the input pointer refers to.

Other APIs in this change may have similar dependency. I suggest we could check their implementation a bit more.

Harbormaster completed remote builds in B115763: Diff 361071.Jul 22 2021, 9:13 PM
gbalats marked an inline comment as done.Jul 23 2021, 12:38 PM
gbalats added inline comments.
compiler-rt/lib/dfsan/done_abilist.txt
76

isatty(3) is a library function and is usually implemented by calling tcgetattr(3) and checking its return value.

Not sure if there might be a direct dependency, because it depends on tcgetattr's implementation. In any case, I wouldn't think file descriptors will have non-zero labels in practice so it probably doesn't make a lot of difference. I could still change it to discard, if you want.

159

Yeah, this is the one I was most unsure about. However, FILE* is an opaque struct. Do you think a custom wrapper that simply copied the file descriptor argument's label to the return label would suffice?

163

I think marking this discard is consistent with other wrappers. E.g., if you look at __dfsw_fgets (or other IO-related functions) it doesn't do anything with its stream_label nor does it propagate it to its output. Since fgetc has only a single stream argument, its implementation would basically just zero out the return label which iiuc is equivalent to marking it discard.

stephan.yichao.zhao added inline comments.Jul 23 2021, 1:00 PM
compiler-rt/lib/dfsan/done_abilist.txt
159

This is how other sanitizers define their interceptors for fdopen. It seems that we do not take FILE* as opaque. We can refer to this file for other wrappers too.

Revision Contents

PathSize
compiler-rt/
lib/
dfsan/
19 lines

Diff 361071

compiler-rt/lib/dfsan/done_abilist.txt

Show First 20 Lines • Show All 67 Lines • ▼ Show 20 Lines
fun:isdigit=functional fun:isdigit=functional
fun:isprint=functional fun:isprint=functional
fun:isxdigit=functional fun:isxdigit=functional
fun:isalnum=functional fun:isalnum=functional
fun:ispunct=functional fun:ispunct=functional
fun:isspace=functional fun:isspace=functional
fun:tolower=functional fun:tolower=functional
fun:toupper=functional fun:toupper=functional
fun:isatty=functional
stephan.yichao.zhaoUnsubmitted
Not Done
ReplyInline Actions

Is this discard?

https://man7.org/linux/man-pages/man3/isatty.3.html

isatty() returns 1 if fd is an open file descriptor referring to a terminal; otherwise 0 is returned, and errno is set to indicate the error.

This sounds like the return value has no direct dependency on fd.

stephan.yichao.zhao: Is this discard? https://man7.org/linux/man-pages/man3/isatty.3.html > isatty() returns 1 if…
gbalatsAuthorUnsubmitted
Done
ReplyInline Actions

isatty(3) is a library function and is usually implemented by calling tcgetattr(3) and checking its return value.

Not sure if there might be a direct dependency, because it depends on tcgetattr's implementation. In any case, I wouldn't think file descriptors will have non-zero labels in practice so it probably doesn't make a lot of difference. I could still change it to discard, if you want.

gbalats: isatty(3) is a library function and is usually implemented by calling tcgetattr(3) and checking…
# Functions that return a value that is data-dependent on the input. # Functions that return a value that is data-dependent on the input.
fun:__isinf=functional fun:__isinf=functional
fun:__isinff=functional fun:__isinff=functional
fun:__signbit=functional fun:__signbit=functional
fun:__signbitf=functional fun:__signbitf=functional
fun:__signbitl=functional fun:__signbitl=functional
fun:abs=functional
fun:btowc=functional fun:btowc=functional
fun:cabs=functional
fun:cabsf=functional
fun:cabsl=functional
fun:exp=functional fun:exp=functional
fun:exp2=functional fun:exp2=functional
fun:expf=functional fun:expf=functional
fun:expl=functional fun:expl=functional
fun:fabs=functional fun:fabs=functional
fun:fabsf=functional
fun:fabsl=functional
fun:finite=functional fun:finite=functional
fun:finitef=functional fun:finitef=functional
fun:finitel=functional fun:finitel=functional
fun:floor=functional fun:floor=functional
fun:fmod=functional fun:fmod=functional
fun:isinf=functional fun:isinf=functional
fun:isinff=functional fun:isinff=functional
fun:isinfl=functional fun:isinfl=functional
fun:isnan=functional fun:isnan=functional
fun:isnanf=functional fun:isnanf=functional
fun:isnanl=functional fun:isnanl=functional
fun:labs=functional
fun:llabs=functional
fun:log=functional fun:log=functional
fun:log1p=functional fun:log1p=functional
fun:log1pf=functional fun:log1pf=functional
fun:log1pl=functional fun:log1pl=functional
fun:log2=functional fun:log2=functional
fun:log2f=functional fun:log2f=functional
fun:log2l=functional fun:log2l=functional
fun:modf=functional fun:modf=functional
Show All 34 Lines
fun:connect=discard fun:connect=discard
fun:creat=discard fun:creat=discard
fun:dladdr=discard fun:dladdr=discard
fun:dlclose=discard fun:dlclose=discard
fun:epoll_create=discard fun:epoll_create=discard
fun:epoll_create1=discard fun:epoll_create1=discard
fun:epoll_ctl=discard fun:epoll_ctl=discard
fun:fclose=discard fun:fclose=discard
fun:fdopen=discard
stephan.yichao.zhaoUnsubmitted
Not Done
ReplyInline Actions

This one probably needs a custom wrapper.

Here is an mac os implementation.
We can see

fp->_file = fd;

So we need to propagate fd's label to fp's _file field. It would be great to find a linux implementation to confirm this.

stephan.yichao.zhao: This one probably needs a custom wrapper. Here is an [[ https://opensource.apple.
gbalatsAuthorUnsubmitted
Done
ReplyInline Actions

Yeah, this is the one I was most unsure about. However, FILE* is an opaque struct. Do you think a custom wrapper that simply copied the file descriptor argument's label to the return label would suffice?

gbalats: Yeah, this is the one I was most unsure about. However, FILE* is an opaque struct. Do you think…
stephan.yichao.zhaoUnsubmitted
Not Done
ReplyInline Actions

This is how other sanitizers define their interceptors for fdopen. It seems that we do not take FILE* as opaque. We can refer to this file for other wrappers too.

stephan.yichao.zhao: [[ https://github.com/llvm/llvm-project/blob/main/compiler…
fun:feof=discard fun:feof=discard
fun:ferror=discard fun:ferror=discard
fun:fflush=discard fun:fflush=discard
fun:fgetc=discard
stephan.yichao.zhaoUnsubmitted
Not Done
ReplyInline Actions

From this uncommon implementation, the return value may depend on the contents the input pointer refers to.

Other APIs in this change may have similar dependency. I suggest we could check their implementation a bit more.

stephan.yichao.zhao: From this uncommon [[http://mirror.fsf.org/pmon2000/2.x/src/lib/libc/fgetc.c |…
gbalatsAuthorUnsubmitted
Done
ReplyInline Actions

I think marking this discard is consistent with other wrappers. E.g., if you look at __dfsw_fgets (or other IO-related functions) it doesn't do anything with its stream_label nor does it propagate it to its output. Since fgetc has only a single stream argument, its implementation would basically just zero out the return label which iiuc is equivalent to marking it discard.

gbalats: I think marking this discard is consistent with other wrappers. E.g., if you look at…
fun:fileno=discard fun:fileno=discard
fun:fopen=discard fun:fopen=discard
fun:fprintf=discard fun:fprintf=discard
fun:fputc=discard fun:fputc=discard
fun:fputc=discard
fun:fputs=discard
fun:fputs=discard fun:fputs=discard
fun:fseek=discard fun:fseek=discard
fun:fseeko=discard
fun:ftell=discard fun:ftell=discard
fun:ftello=discard
fun:fwrite=discard fun:fwrite=discard
fun:getc=discard
fun:getchar=discard
fun:getenv=discard fun:getenv=discard
fun:getuid=discard fun:getuid=discard
fun:geteuid=discard fun:geteuid=discard
fun:getpagesize=discard fun:getpagesize=discard
fun:getpid=discard fun:getpid=discard
fun:kill=discard fun:kill=discard
fun:listen=discard fun:listen=discard
fun:lseek=discard fun:lseek=discard
▲ Show 20 Lines • Show All 147 Lines • ▼ Show 20 Lines
fun:pthread_attr_setguardsize=discard fun:pthread_attr_setguardsize=discard
fun:pthread_attr_setinheritsched=discard fun:pthread_attr_setinheritsched=discard
fun:pthread_attr_setschedparam=discard fun:pthread_attr_setschedparam=discard
fun:pthread_attr_setschedpolicy=discard fun:pthread_attr_setschedpolicy=discard
fun:pthread_attr_setscope=discard fun:pthread_attr_setscope=discard
fun:pthread_attr_setstack=discard fun:pthread_attr_setstack=discard
fun:pthread_attr_setstackaddr=discard fun:pthread_attr_setstackaddr=discard
fun:pthread_attr_setstacksize=discard fun:pthread_attr_setstacksize=discard
fun:pthread_cond_broadcast=discard
fun:pthread_cond_signal=discard
fun:pthread_equal=discard fun:pthread_equal=discard
fun:pthread_getschedparam=discard fun:pthread_getschedparam=discard
fun:pthread_getspecific=discard fun:pthread_getspecific=discard
fun:pthread_key_create=discard fun:pthread_key_create=discard
fun:pthread_key_delete=discard fun:pthread_key_delete=discard
fun:pthread_mutex_destroy=discard fun:pthread_mutex_destroy=discard
fun:pthread_mutex_init=discard fun:pthread_mutex_init=discard
fun:pthread_mutex_lock=discard fun:pthread_mutex_lock=discard
▲ Show 20 Lines • Show All 124 Lines • Show Last 20 Lines