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 jos...@codesourcery.com
