This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATH] Intel offload library
- From: "Joseph S. Myers" <joseph at codesourcery dot com>
- To: Kirill Yukhin <kirill dot yukhin at gmail dot com>
- Cc: Jakub Jelinek <jakub at redhat dot com>, Richard Henderson <rth at redhat dot com>, <bernds at codesourcery dot com>, Richard Biener <rguenther at suse dot de>, Uros Bizjak <ubizjak at gmail dot com>, GCC Patches <gcc-patches at gcc dot gnu dot org>
- Date: Thu, 26 Jun 2014 21:27:31 +0000
- Subject: Re: [PATH] Intel offload library
- Authentication-results: sourceware.org; auth=none
- References: <20140626133400 dot GA32185 at msticlxl57 dot ims dot intel 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