Re: [PATH] Intel offload library

["Joseph S. Myers" <joseph at codesourcery dot com>]

Some observations on this code (the first is about integration in GCC, the 
rest about general good practice for writing robust C libraries for use on 
GNU/Linux, where failure to follow such practice is likely to lead to 
problems and security advisories in future, so if the code goes in 
without these being fixed then I think it would be desirable to have 
clear plans for fixing them before 4.10 branches):

* Don't duplicate the logic for what's a hosted POSIX system; refactor it 
to a common fragment in config/ (I guess it needs to be a shell script 
fragment there rather than an actual autoconf macro, since you're using 
that logic in configure.tgt which is itself such a fragment).

* There seems to be lots of code here that calls malloc or strdup (maybe 
other allocation functions, but at least those two) then dereferences the 
result without checking for errors.  You need to check for errors and then 
either return an error status or throw an exception (depending on what the 
API of the function doing the allocation says is the way it should 
indicate errors).  (There could always be other functions being called 
without error checking; for pretty much any standard C library function 
that can return error status, you need to handle errors appropriately.)

* Code here is using getenv for various purposes.  If it's to be safe to 
use this functionality in setuid programs (and I see no obvious reason why 
setuid programs shouldn't be able to use such offloading), then 
secure_getenv should be used when available (glibc 2.17 and later), with 
the relevant configuration being determined in some safe way when setuid 
or those environment variables aren't set.

* Some code is using strtok.  Is there a reason there can only ever be one 
thread in the process when that code runs?  If not, you shouldn't be using 
strtok; strtok_r, as used elsewhere, is OK.

* Another thread issue: is there a reason there can only be one thread 
when you call fopen?  If not, with glibc you should specify "e" in the 
fopen mode so that the file is opened with O_CLOEXEC, to avoid leaking 
a file descriptor if another thread creates a process while the file is 
open.  Similarly, calls to open should use O_CLOEXEC when available, 
unless you need file descriptors to stay open across exec.

* Generally it's not obvious to me whether the code using strcat, sprintf 
etc. is safe against buffer overruns given arbitrary environment 
variables, valid but extreme function arguments, etc. - I don't know what 
the trust boundaries are in this code, but it would be a good idea to make 
explicit, in code doing such buffer manipulations that are not obviously 
safe, where the responsibility lies for safety (e.g. if the API for a 
function is that arguments outside a certain range yield undefined 
behavior, and it's clear that only outside that range can there be a 
buffer overrun, make clear in any relevant documentation / comments that 
this is the API, and ensure that callers coming with GCC respect that 
API).

* MESSAGE_TABLE_NAME contains a string using 0x91 and 0x92 Windows-1252 
quotes.  It's wrong to use those unconditionally.  If you want to use 
non-ASCII messages you need to respect the user's locale (and if you do, 
supporting translated messages via gettext is a good idea, not just making 
quotes follow LC_CTYPE).

* I suspect your uses of PATH_MAX will cause trouble to anyone trying to 
build this code for Hurd, though I think that can be left to Hurd porters 
to fix.  (More significant is that before blindly putting things in a 
buffer of size PATH_MAX you need a reason that buffer can't overrun - it's 
one thing if you know the path in question has already been successfully 
opened, another if you're building it up by pieces without an API saying 
the caller must ensure those pieces add up to no more than PATH_MAX in 
size, and appropriate code in the callers to ensure this.)

-- 
Joseph S. Myers
joseph@codesourcery.com