systemd-bootchart: Prevent closing random file descriptors

If the kernel has no CONFIG_SCHED_DEBUG option set, systemd-bootchart produces
empty .svg file. The reason for this is very fragile file descriptor logic in
log_sample() and main() (/* do some cleanup, close fd's */ block). There are
many places where file descriptors are closed on failure (missing SCHED_DEBUG
provokes it), but there are several problems with it:
 - following iterations in the loop see that the descriptor is non zero and do
   not open the corresponding file again;
 - "some cleanup" code closes already closed files and the descriptors are reused
   already, in particular for resulting .svg file;
 - static "vmstat" and "schedstat" variables in log_sample() made the situation
   even worse.

These are the strace fragments:

[...]
close(7)                                = -1 EBADF (Bad file descriptor)
close(-1)                               = -1 EBADF (Bad file descriptor)
pread(7, 0xbea60a2c, 4095, 0)           = -1 EBADF (Bad file descriptor)
close(7)                                = -1 EBADF (Bad file descriptor)
close(-1)                               = -1 EBADF (Bad file descriptor)
pread(7, 0xbea60a2c, 4095, 0)           = -1 EBADF (Bad file descriptor)
close(7)                                = -1 EBADF (Bad file descriptor)
close(-1)                               = -1 EBADF (Bad file descriptor)
getdents64(4, /* 0 entries */, 32768)   = 0
clock_gettime(CLOCK_MONOTONIC, {24, 783843501}) = 0
nanosleep({0, 5221792}, NULL)           = 0
clock_gettime(CLOCK_MONOTONIC, {24, 789726835}) = 0
lseek(4, 0, SEEK_SET)                   = 0
pread(5, "nr_free_pages 52309\nnr_alloc_bat"..., 4095, 0) = 685
pread(6, "version 15\ntimestamp 4294939775\n"..., 4095, 0) = 86
getdents64(4, /* 99 entries */, 32768)  = 2680
pread(7, 0xbea60a2c, 4095, 0)           = -1 EBADF (Bad file descriptor)
close(7)                                = -1 EBADF (Bad file descriptor)
close(-1)                               = -1 EBADF (Bad file descriptor)
pread(8, 0xbea60a2c, 4095, 0)           = -1 EBADF (Bad file descriptor)
close(8)                                = -1 EBADF (Bad file descriptor)
close(-1)                               = -1 EBADF (Bad file descriptor)
pread(9, 0xbea60a2c, 4095, 0)           = -1 EBADF (Bad file descriptor)
close(9)                                = -1 EBADF (Bad file descriptor)
[...]

where it obviously tries to close same and reused decriptors many times, also
passing return code "-1" instead of descriptor...

[...]
close(7)                                = -1 EBADF (Bad file descriptor)
close(-1)                               = -1 EBADF (Bad file descriptor)
pipe2([7, 8], O_CLOEXEC)                = 0
clone(child_stack=0, flags=CLONE_CHILD_CLEARTID|CLONE_CHILD_SETTID|SIGCHLD, child_tidptr=0xb6fd0068) = 192
close(8)                                = 0
fcntl64(7, F_SETFD, 0)                  = 0
fstat64(7, {st_mode=S_IFIFO|0600, st_size=0, ...}) = 0
mmap2(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0xb6fd2000
read(7, "[    0.074507] calling  vfp_init"..., 4096) = 4096
[...]
read(7, "s)\n[    6.228910] UBIFS: reserve"..., 4096) = 4096
read(7, "trary Executable File Formats Fi"..., 4096) = 1616
read(7, "", 4096)                       = 0
close(7)                                = 0
wait4(192, [{WIFEXITED(s) && WEXITSTATUS(s) == 0}], 0, NULL) = 192

diff --git a/src/bootchart/bootchart.c b/src/bootchart/bootchart.c
index 71dffc9ae8b3a497ee2fc43cf8ca5b1add8958e2..f50479d0f8f642055a64507a1901db7cb2ae9926 100644 (file)
--- a/src/bootchart/bootchart.c
+++ b/src/bootchart/bootchart.c
@@ -448,9 +448,9 @@ int main(int argc, char *argv[]) {
         ps = ps_first;
         while (ps->next_ps) {
                 ps = ps->next_ps;
-                if (ps->schedstat)
+                if (ps->schedstat >= 0)
                         close(ps->schedstat);
-                if (ps->sched)
+                if (ps->sched >= 0)
                         close(ps->sched);
                 if (ps->smaps)
                         fclose(ps->smaps);

diff --git a/src/bootchart/store.c b/src/bootchart/store.c
index dfa681f223a9bdc468a9862dd09e06b1d6326856..3900936acfc21da4a5338c8660d495f06f6f3f2e 100644 (file)
--- a/src/bootchart/store.c
+++ b/src/bootchart/store.c
@@ -108,8 +108,8 @@ static int pid_cmdline_strscpy(char *buffer, size_t buf_len, int pid) {
 }
 
 void log_sample(int sample, struct list_sample_data **ptr) {
-        static int vmstat;
-        static int schedstat;
+        static int vmstat = -1;
+        static int schedstat = -1;
         char buf[4096];
         char key[256];
         char val[256];
@@ -140,7 +140,7 @@ void log_sample(int sample, struct list_sample_data **ptr) {
                 rewinddir(proc);
         }
 
-        if (!vmstat) {
+        if (vmstat < 0) {
                 /* block stuff */
                 vmstat = openat(procfd, "vmstat", O_RDONLY);
                 if (vmstat == -1) {
@@ -152,6 +152,7 @@ void log_sample(int sample, struct list_sample_data **ptr) {
         n = pread(vmstat, buf, sizeof(buf) - 1, 0);
         if (n <= 0) {
                 close(vmstat);
+                vmstat = -1;
                 return;
         }
         buf[n] = '\0';
@@ -172,7 +173,7 @@ vmstat_next:
                         break;
         }
 
-        if (!schedstat) {
+        if (schedstat < 0) {
                 /* overall CPU utilization */
                 schedstat = openat(procfd, "schedstat", O_RDONLY);
                 if (schedstat == -1) {
@@ -184,6 +185,7 @@ vmstat_next:
         n = pread(schedstat, buf, sizeof(buf) - 1, 0);
         if (n <= 0) {
                 close(schedstat);
+                schedstat = -1;
                 return;
         }
         buf[n] = '\0';
@@ -260,6 +262,8 @@ schedstat_next:
                         }
                         ps = ps->next_ps;
                         ps->pid = pid;
+                        ps->sched = -1;
+                        ps->schedstat = -1;
 
                         ps->sample = new0(struct ps_sched_struct, 1);
                         if (!ps->sample) {
@@ -276,7 +280,7 @@ schedstat_next:
                         ps->sample->waittime = atoll(wt);
 
                         /* get name, start time */
-                        if (!ps->sched) {
+                        if (ps->sched < 0) {
                                 sprintf(filename, "%d/sched", pid);
                                 ps->sched = openat(procfd, filename, O_RDONLY);
                                 if (ps->sched == -1)
@@ -286,6 +290,7 @@ schedstat_next:
                         s = pread(ps->sched, buf, sizeof(buf) - 1, 0);
                         if (s <= 0) {
                                 close(ps->sched);
+                                ps->sched = -1;
                                 continue;
                         }
                         buf[s] = '\0';
@@ -377,7 +382,7 @@ schedstat_next:
                  * iteration */
 
                 /* rt, wt */
-                if (!ps->schedstat) {
+                if (ps->schedstat < 0) {
                         sprintf(filename, "%d/schedstat", pid);
                         ps->schedstat = openat(procfd, filename, O_RDONLY);
                         if (ps->schedstat == -1)
@@ -387,8 +392,11 @@ schedstat_next:
                 if (s <= 0) {
                         /* clean up our file descriptors - assume that the process exited */
                         close(ps->schedstat);
-                        if (ps->sched)
+                        ps->schedstat = -1;
+                        if (ps->sched) {
                                 close(ps->sched);
+                                ps->sched = -1;
+                        }
                         //if (ps->smaps)
                         //        fclose(ps->smaps);
                         continue;
@@ -488,8 +496,11 @@ catch_rename:
                         if (s <= 0) {
                                 /* clean up file descriptors */
                                 close(ps->sched);
-                                if (ps->schedstat)
+                                ps->sched = -1;
+                                if (ps->schedstat) {
                                         close(ps->schedstat);
+                                        ps->schedstat = -1;
+                                }
                                 //if (ps->smaps)
                                 //        fclose(ps->smaps);
                                 continue;