[systemd-devel] [PATCH 4/4] coredump: collect all /proc data useful for bug reporting

[Lennart Poettering]

On Wed, 19.11.14 11:01, Jakub Filak (jfilak at redhat.com) wrote:

> +/* Joins /proc/[pid]/fd/ and /proc/[pid]/fdinfo/ into the following lines:
> + *
> + * 0:/dev/pts/23
> + * pos:    0
> + * flags:  0100002
> + * 1:/dev/pts/23
> + * pos:    0
> + * flags:  0100002
> + * 2:/dev/pts/23

Hmm, I'd prefer a format here that is more easily reversible. For
example, adding an extra newline between the fdinfo items would be a
good start.

> + *
> + */
> +static int compose_open_fds(pid_t pid, char **open_fds) {
> +        const char *fd_name = NULL, *fdinfo_name = NULL;

const? why?

> +        char *outcome = NULL;
> +        size_t len = 0, allocated = 0;
> +        char line[LINE_MAX];
> +        unsigned fd = 0;
> +        int r = 0;
> +
> +        assert(pid >= 0);
> +
> +        fd_name = alloca(strlen("/proc//fd/") + DECIMAL_STR_MAX(pid_t) + DECIMAL_STR_MAX(unsigned) + 1);
                                                                                            ^^^
                                                                                            unsigned? an fd is an int!  

> +        fdinfo_name = alloca(strlen("/proc//fdinfo/") + DECIMAL_STR_MAX(pid_t) + DECIMAL_STR_MAX(unsigned) + 1);

same here.

The sizes you allocate here are fixed. I'd really prefer if you'd
allocate these as normal arrays instead of alloca(). alloca() is a
useful tool, but we should use it only when normal arrays aren't good
denough, but not otherwise.

Also note that alloca() cannot be mixed with function calls in the
same line. strlen() is a function call (though one that today's gcc
actually is smart enough to optimize away at compile time if you
invoke it on a literal string). 

Hence, please use this instead:

char fd_name[sizeof("/proc/")-1 + DECIMAL_STR_MAX(pid_t) + sizeof("/fd/")-1 + DECIMAL_STR_MAX(int) + 1];

> +
> +        while (fd <= 99999) {

Oh no, this is not OK!

We shouldn't iterate though all thinkable fds, that's bad code. Please
iterate through /proc/$PID/fd/ and just operate on fds that are
actually there.

> +                _cleanup_free_ char *name = NULL;
> +                _cleanup_fclose_ FILE *fdinfo = NULL;
> +
> +                sprintf((char *)fd_name, "/proc/"PID_FMT"/fd/%u", pid, fd);

Hmm, first you declare the string as "const", then you cast this away?
This is usually a good indication that something is really wrong...

> +                r = readlink_malloc(fd_name, &name);
> +                if (r < 0) {
> +                        if (r == -ENOENT) {
> +                            *open_fds = outcome;
> +                            r = 0;
> +                        }
> +                        else
> +                            free(outcome);
> +
> +                        break;
> +                }
> +
> +                if (!GREEDY_REALLOC(outcome, allocated, len + strlen(name) + DECIMAL_STR_MAX(unsigned) + 3))
> +                        return -ENOMEM;
> +
> +                len += sprintf(outcome + len, "%u:%s\n", fd, name);
> +                ++fd;
> +
> +                sprintf((char *)fdinfo_name, "/proc/"PID_FMT"/fdinfo/%u", pid, fd);
> +                fdinfo = fopen(fdinfo_name, "r");
> +                if (fdinfo == NULL)
> +                        continue;
> +
> +                while(fgets(line, sizeof(line), fdinfo) != NULL) {
> +                        if (!GREEDY_REALLOC(outcome, allocated, len + strlen(line) + 2))
> +                                return -ENOMEM;
> +
> +                        len += sprintf(outcome + len, "%s", line);
> +                        if (strchr(line, '\n') == NULL) {
> +                                outcome[len++] = '\n';
> +                                outcome[len] = '\0';
> +                        }

> +                }

I think using libc's open_memstream() and then simply writing to it
would be a *ton* prettier than this.

Lennart

-- 
Lennart Poettering, Red Hat