Opened 8 years ago

Closed 20 months ago

#1593 closed task (fixed)

Improve runInteractiveProcess error message when working directory does not exist

Reported by: tomasz.zielonka@… Owned by: simonmar
Priority: low Milestone:
Component: libraries/process Version: 6.7
Keywords: Cc: tomasz.zielonka@…, marig1@…, igloo
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: None/Unknown Test Case: process004
Blocked By: Blocking:
Related Tickets: Differential Revisions:

Description

Example code:

  import System.Process

  main = do
      (_, _, _, commhand) <-
          runInteractiveProcess "echo" ["something"] (Just "/no/such/dir") Nothing
      exitCode <- waitForProcess commhand
      print exitCode

This happens on Linux with both 6.6 and the latest HEAD snapshot.

I think the problem is in C code, in libraries/base/cbits/runProcess.c (6.6).
Below are the important code fragments. Search for BUG HERE for a bug location.

ProcHandle
runInteractiveProcess (char *const args[],
               char *workingDirectory, char **environment,
               int *pfdStdInput, int *pfdStdOutput, int *pfdStdError)
{
    ...

    switch(pid = fork())
    {
    ...

    case 0:
    {
        pPrPr_disableITimers();

        if (workingDirectory) {
            if (chdir (workingDirectory) < 0) {
                // BUG HERE:
                // Calling return here is bad idea here. This code
                // is run in the child process and returning makes the
                // child act as if it was the parent - strange things will
                // happen!
                return -1;
            }
        }

        ...

        /* the child */
        if (environment) {
            execvpe(args[0], args, environment);
        } else {
            execvp(args[0], args);
        }
    }
    _exit(127);

    default:
    ...

I thought about sending a patch, but I'm not sure what the fix should be. Doing "_exit(SOMETHING)" instead of return seems to be better then returning, but I don't know what SOMETHING should be, and maybe there is a better solution.

I think I've seen the same problem in another run*Process function in this C file.

Change History (15)

comment:1 Changed 8 years ago by guest

I forgot to show how the example program behaves:

$ ./B
B: echo: runInteractiveProcess: does not exist (No such file or directory)

Segmentation fault (core dumped)

comment:2 Changed 8 years ago by igloo

  • Milestone set to 6.8
  • Priority changed from normal to high

We should certainly do something for 6.8.1, even if it's just _exit(1).

comment:3 Changed 8 years ago by simonmar

  • Owner set to simonmar

I'll do this next

comment:4 Changed 8 years ago by simonmar

  • Resolution set to fixed
  • Status changed from new to closed
  • Test Case set to process004

Fixed:

Fri Aug 24 16:46:53 BST 2007  Simon Marlow <[email protected]>
  * FIX #1593: failing to set the working directory results in exit(126) now

comment:5 Changed 7 years ago by igloo

  • Milestone changed from 6.8 branch to 6.8.1

comment:6 Changed 7 years ago by simonmar

  • Component changed from libraries (other) to libraries/process

comment:7 follow-up: Changed 7 years ago by wferi

Just some thoughts. On Posix systems, you could do something like this, if you can spare a file descriptor:

int parentCWD;

if (workingDirectory) {
    parentCWD = dirfd (opendir ("."));
    if (chdir (workingDirectory)) < 0) {
        return -1; /* raise the proper exception to the caller */
    }

switch (pid = fork ())
{
...

case 0:
{
    pPrPr_disableITimers();

    /* leave out the directory bussiness here */
 
    /* the child */
    if (environment) {
        execvpe(args[0], args, environment);
    } else {
        execvp(args[0], args);
    }
}
_exit(127);

default:
if (workingDirectory) {
    if (fchdir (parentCWD)) {
        /* this can hardly fail, but if it does, throw an exception */
        close (parentCWD);
        return -1;
    }
    close (parentCWD);
}
...
}

I've got no idea about Windows.

comment:8 Changed 7 years ago by simonmar

  • Milestone changed from 6.8.1 to _|_
  • Priority changed from high to low
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Summary changed from runInteractiveProcess misbehaves when it gets invalid (eg. non-existent) working directory parameter to Improve runInteractiveProcess error message when working directory does not exist
  • Type changed from bug to task

Re-opening as a low-priority task.

comment:9 in reply to: ↑ 7 Changed 7 years ago by wferi

Just don't forget closing parentCWD in the child, before exec. Like I did...

comment:10 Changed 6 years ago by simonmar

  • Architecture changed from Unknown to Unknown/Multiple

comment:11 Changed 6 years ago by simonmar

  • Operating System changed from Unknown to Unknown/Multiple

comment:12 Changed 2 years ago by morabbin

  • Type of failure set to None/Unknown

Bump; still relevant?

comment:13 Changed 2 years ago by simonmar

Still needs fixing, yes.

comment:14 Changed 20 months ago by simonmar

  • Cc igloo added

@igloo: this is fixed now, right?

comment:15 Changed 20 months ago by igloo

  • Resolution set to fixed
  • Status changed from new to closed

Yes; we now get:

q: echo: runInteractiveProcess: runInteractiveProcess: chdir: does not exist (No such file or directory)
Note: See TracTickets for help on using tickets.