Safe & correct versions of procuall, proclos

Hi Christof,

Thanks for your feedback! I agree that writing safe C is hard, I find
it quite a fun exercise though. It's also fun to see how engaged
people get when talking about some of these problems.

>
>There are a couple of problems in your code:
>
>1. you don't free the "out" and "line" buffers.
Yep, the fact that the program is just a one-shot test that exits immediately
meant that I was less worried about memory leaks.

>
>2. actually, there is no reason to malloc "out" in the first place; just
>put the array on the stack with "char out[MAXLINE];"

This is a valid point, actually stack allocation works fine for both
"fname" and "out".

>3. I'm not sure why you are calling "fileno" and "fstat". You don't seem
>to use the result, so maybe you're just checking whether the file
>exists? In that case, "fopen" would have failed already. Just seems
>entirely redundant to me

Ok, so I understand your confusion. The program is written to help me
better understand a particular type of error - attempting a read of
a "proc" pseudo file after the process has disappeared. The calls to
"fileno" (to get the file descriptor) and "fstat" to initialize the
"struct stat sb" are only interesting in the case that the program
succeeds. They're not really "doing" anything as such, just included to
indicate that a real program would presumably go off and do some work
after the fopen call.

It should be possible to run the program successfully with
"./proclos $$ 1"

>
>4. "sprintf" is unsafe, always use "snprintf" instead!
Thanks for the tip!
>
>5. "sprintf" (and "snprintf") already return the number of written
>characters - no need for "strlen(out)"!
Cheers.
>
>6. You never call "fclose"
Changed this.
>
>7. There's is no proper cleanups on early returns
Yeah, so I think this should be better in the corrected version.
Actually, I had already implemented this in "procuall" yesterday, but of
course Murphy's law meant that you looked at the other program!
>
>8. Instead writing to a string buffer with "sprintf" and then writing
>that buffer to stdout with "fwrite", you could just use "fprintf" :)
Yep, this also makes much more sense. I think I was just copy-pasta-ing
from the getline example in the linux man pages.

>
>I was only looking at "proclos.c". I assume "procuall.c" has similar errors.
Yeah, I had started to address the issues around cleanup in
"procuall.c". Anyway, both versions have now included your suggestions
and I'm happy to report both are leak-free!

>
>Writing proper C is *hard*, in particular error handling and resource
>cleanup. That's where C++'s RAII really shines.
I found that writing a lot of assembly helped to understand better how
to use "goto" statements for this type of thing. It's amazing how much
easier it is to write high level languages after wrestling with this
type of problem. I haven't written any C++ recently, part of me is
itching to try out either Rust or Zig though.

>
>It's a good exercise, though!
>

proc/proclos.c

@@ -36,10 +36,10 @@ int main(int argc, char* argv[]) {
     if (!s_isinteger(pid)) {
         fprintf(stderr, "Error: invalid pid %s\n", pid);
         exit(EPERM);
-    } 
+    }
     /* Otherwise set up the path we want to read */
-    char fname[FILENAME_MAX];
-    sprintf(fname, "/proc/%s/status", pid);
+    char fname[MAXLINE];
+    snprintf(fname, MAXLINE, "/proc/%s/status", pid);
 
     int time;
     time = atoi(argv[2]);
@@ -55,35 +55,36 @@ int main(int argc, char* argv[]) {
     char* line = NULL;
     size_t len = 0;
     ssize_t rread = 0;
-    char* out = NULL;
     struct stat sb;
 
     dirp = opendir(PROC);
     if (dirp) {
         errno = 0;
         if ((dp = readdir(dirp)) != NULL) {
             sleep(time); /* Sleep while the process gets killed */
-            fp = fopen(fname, "r");
-            if (fp == NULL) {
+
+            if ((fp = fopen(fname, "r")) == NULL) {
                 fprintf(stderr, "Error: fopen failed to complete with code %d\n", errno);
-                exit(ESRCH); /* Process not found */
+                goto err_file;
             }
-            fd = fileno(fp);
-            if (fstat(fd, &sb) == -1) {
-                return -1; /* just cheese it! */
+
+            if ((fd = fileno(fp)) == -1) {
+                fprintf(stderr, "Error fileno %d\n", errno);
+                goto err_proc;
             }
-            out = (char*)malloc(MAXLINE);
-            if (out == NULL) {
-                fprintf(stderr, "Error: malloc\n");
-                goto err_malloc;
+
+            if (fstat(fd, &sb) == -1) {
+                fprintf(stderr, "Error fstat %d\n", errno);
+                goto err_proc;
             }
+
             if ((rread = getline(&line, &len, fp)) != -1) {
-                sprintf(out, "%-24s pid:%-30.30s\n", line, pid);
-                int llen = strlen(out);
-                fwrite(out, llen, 1, stdout);
+                fprintf(stdout, "%spid:\t%s\n", line, pid);
             }
         }
-err_malloc:
+err_proc:
+        fclose(fp);
+err_file:
         closedir(dirp);
     }
     return 0;

proc/procuall.c

@@ -25,10 +25,11 @@ bool s_isinteger(const char* s) {
     }
     return result;
 }
+
 int
-make_filename(char* fname, const char* pid) {
+make_filename(char fname[MAXLINE], const char* pid) {
     if (s_isinteger(pid)) {
-        sprintf(fname, "/proc/%s/status", pid);
+        snprintf(fname, MAXLINE, "/proc/%s/status", pid);
         return 0;
     }
     return -1;
@@ -49,24 +50,21 @@ main (int argc, char* argv[]) {
     FILE* fp;
     int fd;
 
-    char* fname = NULL;
     char* line = NULL;
-    char* out = NULL;
     ssize_t rread = 0;
     size_t len = 0;
-    ssize_t lout = 0;
     struct stat sb;
 
     dirp = opendir(PROC);
     if (dirp) {
         errno = 0;
         while ((dp = readdir(dirp)) != NULL) {
 
-            fname = (char*)malloc(FILENAME_MAX);
             int rc;
+            char fname[MAXLINE];
             if ((rc = make_filename(fname, dp->d_name)) == -1) {
-                fprintf(stderr, "Error make_filename %d\n", errno);
-                goto err_file;
+                fprintf(stderr, "Error make_filename\n");
+                goto err_fname;
             }
 
             if (access(fname, F_OK) != 0) {
@@ -80,34 +78,29 @@ main (int argc, char* argv[]) {
             }
 
             if ((fd = fileno(fp)) == -1) {
-                fprintf(stderr, "Error fileno %d", errno);
+                fprintf(stderr, "Error fileno %d\n", errno);
                 fclose(fp);
                 goto err_proc;
             }
 
             if (fstat(fd, &sb) == -1) {
-                fprintf(stderr, "Error fstat %d", errno);
+                fprintf(stderr, "Error fstat %d\n", errno);
                 goto err_proc;
             }
 
             if (uid == sb.st_uid) {
                 if ((rread = getline(&line, &len, fp)) != -1) {
-                    out = (char*)malloc(MAXLINE);
-                    sprintf(out, "%spid:\t%s\n", line, dp->d_name);
-                    lout = strlen(out);
-                    fwrite(out, lout, 1, stdout);
-                    free(out);
+                    fprintf(stderr, "%spid:\t%s\n", line, dp->d_name);
                 } else {
-                    fprintf(stderr, "Error letline %d", errno);
+                    fprintf(stderr, "Error getline %d\n", errno);
                 }
             }
 err_proc:
             fclose(fp);
-err_file:
-            free(fname);
         }
+err_fname:
+err_file:
         /* cleanup directory stuff */
-        free(dp);
         closedir(dirp);
     }
     return 0;