LinuxQuestions.org - use of "readline" in conky source code

I've recently gotten into the kick of inspecting source code on my system for bugs and vulnerabilities. Picked conky to start with, basically just because I thought it would be smaller and easier to understand. Anyway, I ran flawfinder on it, and the first thing that popped up was this:

Code:

./core.c:99:  [5] (race) readlink:

  This accepts filename arguments; if an attacker can move those files

  or change the link content, a race condition results.  Also, it does not

  terminate with ASCII NUL. Reconsider approach.

Of course, not all flawfinder reports are real problems, so I went to the source, and found it in the function dev_name:

Code:

/* strip a leading /dev/ if any, following symlinks first                                                        

 *                                                                                                              

 * BEWARE: this function returns a pointer to static content                                                    

 *        which gets overwritten in consecutive calls. I.e.:                                                    

 *        this function is NOT reentrant.                                                                      

 */

const char *dev_name(const char *path)

{

    static char buf[255];  /* should be enough for pathnames */

    ssize_t buflen;



    if (!path)

        return NULL;



#define DEV_NAME(x) \

  x != NULL && strlen(x) > 5 && strncmp(x, "/dev/", 5) == 0 ? x + 5 : x

    if ((buflen = readlink(path, buf, 254)) == -1)

        return DEV_NAME(path);

    buf[buflen] = '\0';

    return DEV_NAME(buf);

#undef DEV_NAME

}

So, it appears that the function dev_name is used in the rest of the code to pre-process a file path argument that is supposed to point to a dev file. For the most part all the function is supposed to do is strip "/dev/" from the file name; however, it also calls the readlink function to determine if the path refers to a symbolic link, in which case dev_name will return the file the link points toward rather than simply the modified version of the original path.

So, a few concerns come to mind:

1. Race condition? Is this a good place to be calling readlink?
2. Necessary? What is wrong with simply using the path to the symbolic link?
3. Bad error handling? Notice that, if readlink returns -1 (error) then dev_name simply forgets about the symbolic link and returns the modified version of the original path. However, READLINK(2) indicates that there are a quite a number of reasons that a readlink call might fail:

Code:

RETURN VALUE

      On  success,  readlink() returns the number of bytes placed in buf.  On error, -1 is returned and errno is set to indi‐

      cate the error.



ERRORS

      EACCES Search permission is denied for a component of the path prefix.  (See also path_resolution(7).)



      EFAULT buf extends outside the process's allocated address space.



      EINVAL bufsiz is not positive.



      EINVAL The named file is not a symbolic link.



      EIO    An I/O error occurred while reading from the file system.



      ELOOP  Too many symbolic links were encountered in translating the pathname.



      ENAMETOOLONG

              A pathname, or a component of a pathname, was too long.



      ENOENT The named file does not exist.



      ENOMEM Insufficient kernel memory was available.



      ENOTDIR

              A component of the path prefix is not a directory.

So it seems like we are just "moving on" under the assumption that the file is not a symbolic link, when there could be some other problem. Maybe would should be exiting the program, or logging an error, or something?

Anyway, before I go throwing my ignorant bug reports at the developer, I was curious for feedback.