Fix clang warning about uninitialized variables

[kainjow]

If pipe() fails, close() could be used on junk std*_p data.

[kainjow]

Ideally, a RAII file descriptor wrapper should be used, so close() doesn't have to be called for each error condition. Maybe I'll put that together later.

[eidheim]

RAII wrapper might maybe make things easier, but a release method would also be needed since not all file descriptors should be closed. My initial thought on this is that it is more readable now, but not 100% sure.

[eidheim]

I think the solution here is to apply the following diff:

diff --git a/process_unix.cpp b/process_unix.cpp
index 37a08bf..e641b67 100644
--- a/process_unix.cpp
+++ b/process_unix.cpp
@@ -16,19 +16,15 @@ Process::id_type Process::open(const std::string &command, const std::string &pa

   int stdin_p[2], stdout_p[2], stderr_p[2];

-  if(stdin_fd && pipe(stdin_p)!=0) {
-    close(stdin_p[0]);close(stdin_p[1]);
+  if(stdin_fd && pipe(stdin_p)!=0)
     return -1;
-  }
   if(stdout_fd && pipe(stdout_p)!=0) {
     if(stdin_fd) {close(stdin_p[0]);close(stdin_p[1]);}
-    close(stdout_p[0]);close(stdout_p[1]);
     return -1;
   }
   if(stderr_fd && pipe(stderr_p)!=0) {
     if(stdin_fd) {close(stdin_p[0]);close(stdin_p[1]);}
     if(stdout_fd) {close(stdout_p[0]);close(stdout_p[1]);}
-    close(stderr_p[0]);close(stderr_p[1]);
     return -1;
   }

since the argument array is probably not set when pipe fails, although I could not find anything in the pipe-documentation. What do you think?

[kainjow]

I think that looks good. although IMHO I wouldn't get rid of the opening/closing curly braces on the if statement just because the if body is a single statement, but that's my personal preference :)

[eidheim]

Thank you, this issue should then be fixed in https://github.com/eidheim/tiny-process-library/commit/8025c4582384333afd22c988861b70cddae0e633

[kainjow]

I still get the same issue after updating.

The problem is just checking the same condition that creates the pipe isn't enough, because pipe() could fail leaving the file descriptor values untouched, e.g. junk from the stack.

If you just initialize the values either via = {0} or memset() then the warnings go away. And then no need to check flags before closing. Could be even safer and write a close wrapper that only closes if the file descriptor is > 0 but that's probably unnecessary as the OS will handle 0'd values.

[eidheim]

What options do you pass to clang analyzer? I use clang analyzer in https://github.com/eidheim/tiny-process-library/blob/master/.travis.yml (scan-build), but without any options. I could test with a newer version of clang, since I do not think Function call argument is an uninitialized value is a correct issue in this case. That is, if pipe does not fail, the values are set, and I think clang-analyzer reports a false positive here.

[kainjow]

Sorry for the delay. I'm calling it through Xcode's gui via Product > Analyze in the menu bar, which doesn't have any options.

[kainjow]

Ok, I agree, I think it's a false positive. It may just be that the analyzer doesn't know that pipe() modifies its arguments.