Fix bad file descriptor error when ignore_output is enabled #1102

cristgl · 2018-08-14T12:36:02Z

This PR fixes the issues detected at #1057.

Also, STDERR, STDOUT and STDIN have been opened with O_RDWR permissions so that there is no bad file descriptor error when ignore_output is active.

vikman90

Please apply the same stander input/output binds to the wpopen library.

Two options to solve the timeout problem:

If output is disabled, use the classical kill loop (https://github.com/wazuh/wazuh/blob/master/src/wazuh_modules/wm_exec.c#L484-L503)
Replace wm_exec() completely for wm_popen() and add this kill loop.

Don't forget to update the changelog.

Thanks @cristgl !

vikman90 · 2018-08-15T20:09:54Z

src/wazuh_modules/wm_exec.c

+            }
+            dup2(fd, STDOUT_FILENO);
+            dup2(fd, STDERR_FILENO);
+            dup2(fd, STDIN_FILENO);


The stdin should also be bound in the then part of the if.

vikman90 · 2018-08-15T20:12:24Z

src/wazuh_modules/wm_exec.c

-            case 0:
-                retval = 0;
-                break;
+       switch (secs ? pthread_cond_timedwait(&tinfo.finished, &tinfo.mutex, &timeout) : pthread_cond_wait(&tinfo.finished, &tinfo.mutex)) {


This part has no sense since we have not created a pipe. This thread may fail and return immediately, so the timeout could not be computed.

vikman90 · 2018-08-17T06:47:54Z

src/wazuh_modules/wm_exec.c

@@ -343,24 +349,46 @@ int wm_exec(char *command, char **output, int *exitcode, int secs)

            pthread_mutex_destroy(&tinfo.mutex);
            pthread_cond_destroy(&tinfo.finished);
+
+            // Wait for child process


waitpid() must be called in any case, otherwise the child process will remain as zombie.

vikman90 · 2018-08-17T06:48:28Z

src/shared/exec_op.c

@@ -128,32 +128,40 @@ wfd_t * wpopenv(const char * path, char * const * argv, int flags) {
        // Error
        break;

-    case 0:
+        case 0:


This indentation...?

vikman90 · 2018-08-17T06:49:35Z

src/wazuh_modules/wm_exec.c

-        }
+            // Kill and timeout
+            do {
+                sleep(1);


Please let's sleep after checking.

vikman90 · 2018-08-17T06:50:02Z

src/wazuh_modules/wmodules.h

@@ -93,7 +93,7 @@ void wm_destroy();
 * If the called program timed-out, returns WM_ERROR_TIMEOUT and output may
 * contain data.
 */
-int wm_exec(char *command, char **output, int *exitcode, int secs);
+int wm_popen(char *command, char **output, int *exitcode, int secs);


Why did you refactor this function?

vikman90 · 2018-08-17T18:22:57Z

src/shared/exec_op.c

-            close(STDOUT_FILENO);
-            close(STDERR_FILENO);
+            dup2(fd, STDOUT_FILENO);
+            dup2(fd, STDERR_FILENO);
        }

        close(STDIN_FILENO);


This line should not be there.

vikman90 · 2018-08-17T18:23:48Z

src/wazuh_modules/wm_exec.c

+                        case -1:
+                            switch(errno){
+                                case ESRCH:
+                                    exit(EXIT_SUCCESS);


This code runs in the parent process. This line would close the daemon.

vikman90 · 2018-08-17T18:26:03Z

src/wazuh_modules/wmodules.c

@@ -112,18 +112,20 @@ int wm_check() {
            next = j->next;

            if (i->context->name == j->context->name) {
-                mdebug1("Deleting repeated module '%s'.", j->context->name);
+                if(strcmp(j->context->name,"command")){


This change fixes a different issue but it would allow repeated modules.

…n one wodle command

vikman90 · 2018-08-20T16:01:15Z

src/wazuh_modules/wm_exec.c

        break;

    default:

        // Parent

        wm_append_sid(pid);
+        close(pipe_fd[1]);


If the output is null, then pipe_fd has trash values.

vikman90 · 2018-08-20T16:03:20Z

src/shared/exec_op.c

        }

-        close(STDIN_FILENO);


We must close fd after duplicating it.

vikman90 · 2018-08-20T17:27:47Z

LGTM!
Thanks @cristgl !

cristgl requested a review from vikman90 August 14, 2018 12:36

cristgl force-pushed the fix-wodle-command branch from 1b83350 to c675e45 Compare August 14, 2018 12:42

cristgl added this to Needs review in Wazuh 3.5.1 via automation Aug 14, 2018

vikman90 requested changes Aug 15, 2018

View reviewed changes

vikman90 self-assigned this Aug 15, 2018

vikman90 added the type/bug Something isn't working label Aug 15, 2018

cristgl force-pushed the fix-wodle-command branch from dfb37d7 to cdfc437 Compare August 17, 2018 05:43

vikman90 requested changes Aug 17, 2018

View reviewed changes

cristgl force-pushed the fix-wodle-command branch 2 times, most recently from d53b2eb to e0f8d3a Compare August 17, 2018 11:33

vikman90 requested changes Aug 17, 2018

View reviewed changes

cristgl and others added 7 commits August 20, 2018 17:53

Fix bad file descriptor error when ignore_output is enabled

dd7c5e1

Fix command timeout when ignore_output is disabled and allow more tha…

c67e192

…n one wodle command

Add /dev/null file descriptor to wpopen library

f65f848

Fix command timeout with ignore_output enabled

0f4ebc6

Fix some error messages

14a5c84

Fix indentation in switch sentence

cb4cf71

Close /dev/null descriptor after binding

d5af270

vikman90 requested changes Aug 20, 2018

View reviewed changes

Do not close pipe descriptor if it was not opened

c88c433

vikman90 force-pushed the fix-wodle-command branch from fad108d to c88c433 Compare August 20, 2018 16:15

vikman90 approved these changes Aug 20, 2018

View reviewed changes

Wazuh 3.5.1 automation moved this from Needs review to Reviewer approved Aug 20, 2018

vikman90 merged commit 55b36fa into 3.5 Aug 20, 2018

Wazuh 3.5.1 automation moved this from Reviewer approved to Done Aug 20, 2018

vikman90 deleted the fix-wodle-command branch August 20, 2018 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bad file descriptor error when ignore_output is enabled #1102

Fix bad file descriptor error when ignore_output is enabled #1102

cristgl commented Aug 14, 2018 •

edited

vikman90 left a comment •

edited

vikman90 Aug 15, 2018

vikman90 Aug 15, 2018

vikman90 Aug 17, 2018

vikman90 Aug 17, 2018

vikman90 Aug 17, 2018

vikman90 Aug 17, 2018

vikman90 Aug 17, 2018

vikman90 Aug 17, 2018

vikman90 Aug 17, 2018

vikman90 Aug 20, 2018

vikman90 Aug 20, 2018

vikman90 commented Aug 20, 2018

Fix bad file descriptor error when ignore_output is enabled #1102

Fix bad file descriptor error when ignore_output is enabled #1102

Conversation

cristgl commented Aug 14, 2018 • edited

vikman90 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vikman90 commented Aug 20, 2018

cristgl commented Aug 14, 2018 •

edited

vikman90 left a comment •

edited