Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added child_user and child_group to CreateProcess for unix. #45

Merged
merged 4 commits into from
Oct 15, 2015
Merged

Added child_user and child_group to CreateProcess for unix. #45

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bc4e5e6
Added child_user and child_group to CreateProcess for unix.
jprider63 Oct 14, 2015
932fe34
Addresses comments from #45. Feel free to squash.
jprider63 Oct 14, 2015
296535e
Revert "Addresses comments from #45. Feel free to squash."
jprider63 Oct 14, 2015
2397d4f
Updated `CreateProcess` documentation after revert.
jprider63 Oct 14, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions System/Process.hsc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,9 @@ proc cmd args = CreateProcess { cmdspec = RawCommand cmd args,
delegate_ctlc = False,
detach_console = False,
create_new_console = False,
new_session = False }
new_session = False,
child_group = Nothing,
child_user = Nothing }

-- | Construct a 'CreateProcess' record for passing to 'createProcess',
-- representing a command to be passed to the shell.
Expand All @@ -142,7 +144,9 @@ shell str = CreateProcess { cmdspec = ShellCommand str,
delegate_ctlc = False,
detach_console = False,
create_new_console = False,
new_session = False }
new_session = False,
child_group = Nothing,
child_user = Nothing }

{- |
This is the most general way to spawn an external process. The
Expand Down
21 changes: 19 additions & 2 deletions System/Process/Internals.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,19 @@ data CreateProcess = CreateProcess{
-- Default: @False@
--
-- @since 1.3.0.0
new_session :: Bool -- ^ Use posix setsid to start the new process in a new session; does nothing on other platforms.
new_session :: Bool, -- ^ Use posix setsid to start the new process in a new session; does nothing on other platforms.
--
-- @since 1.3.0.0
child_group :: Maybe GroupID, -- ^ Use posix setgid to set child process's group id; does nothing on other platforms.
--
-- Default: @Nothing@
--
-- @since 1.4.0.0
child_user :: Maybe UserID -- ^ Use posix setuid to set child process's user id; does nothing on other platforms.
--
-- Default: @Nothing@
--
-- @since 1.4.0.0
}

data CmdSpec
Expand Down Expand Up @@ -273,7 +283,9 @@ createProcess_ fun CreateProcess{ cmdspec = cmdsp,
delegate_ctlc = mb_delegate_ctlc,
detach_console = mb_detach_console,
create_new_console = mb_create_new_console,
new_session = mb_new_session }
new_session = mb_new_session,
child_group = mb_child_group,
child_user = mb_child_user }
= do
let (cmd,args) = commandToProcess cmdsp
withFilePathException cmd $
Expand All @@ -283,6 +295,8 @@ createProcess_ fun CreateProcess{ cmdspec = cmdsp,
alloca $ \ pFailedDoing ->
maybeWith withCEnvironment mb_env $ \pEnv ->
maybeWith withFilePath mb_cwd $ \pWorkDir ->
maybeWith with mb_child_group $ \pChildGroup ->
maybeWith with mb_child_user $ \pChildUser ->
withMany withFilePath (cmd:args) $ \cstrs ->
withArray0 nullPtr cstrs $ \pargs -> do

Expand All @@ -301,6 +315,7 @@ createProcess_ fun CreateProcess{ cmdspec = cmdsp,
c_runInteractiveProcess pargs pWorkDir pEnv
fdin fdout fderr
pfdStdInput pfdStdOutput pfdStdError
pChildGroup pChildUser
(if mb_delegate_ctlc then 1 else 0)
((if mb_close_fds then RUN_PROCESS_IN_CLOSE_FDS else 0)
.|.(if mb_create_group then RUN_PROCESS_IN_NEW_GROUP else 0)
Expand Down Expand Up @@ -414,6 +429,8 @@ foreign import ccall unsafe "runInteractiveProcess"
-> Ptr FD
-> Ptr FD
-> Ptr FD
-> Ptr CGid
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that a CGid or CUid is just an int, I think this can be done without the pointer indirection, by using -1 to represent "not provided."

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this change in the latest commit, but I'm worried that there could be conflicts since uid_t and gid_t are unsigned. It might be better to go with the original.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The research I did before making my comment implied that:

  1. Signedness is implementation specific, and will vary by platform
  2. Even on platforms with an unsigned type, the -1 value is considered reserved

I could be mistaken on this, however. I'm OK with either approach, it just struck me as strange to use a pointer for integral data.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that signedness will vary by platform. Do you have a reference that says -1 is reserved? My concern was that on an unsigned system, -1 could be interpreted as max_int (which although unlikely could be a user's/group's id).

I suppose another option would be to add another mask to the flag indicating whether the set the user/group or not. Maybe this is the safest approach.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I've found:

http://pubs.opengroup.org/onlinepubs/009695399/functions/chown.html
http://stackoverflow.com/questions/21370094/uid-t-type-is-signed-or-not

I wouldn't bother with a mask on the flag, I'd either stick with your original pointer approach or use -1.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like using -1 is probably ok, but I'm still skeptical since setgid's and setuid's documentation don't mention it. I'll revert to the pointer approach since I think it's the safer option.

-> Ptr CUid
-> CInt -- reset child's SIGINT & SIGQUIT handlers
-> CInt -- flags
-> Ptr CString
Expand Down
24 changes: 24 additions & 0 deletions cbits/runProcess.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,10 @@ extern void unblockUserSignals(void);
#define forkChdirFailed 126
#define forkExecFailed 127

// These are arbitrarily chosen -- JP
#define forkSetgidFailed 124
#define forkSetuidFailed 125

__attribute__((__noreturn__))
static void childFailed(int pipe, int failCode) {
int err;
Expand All @@ -57,6 +61,7 @@ runInteractiveProcess (char *const args[],
char *workingDirectory, char **environment,
int fdStdIn, int fdStdOut, int fdStdErr,
int *pfdStdInput, int *pfdStdOutput, int *pfdStdError,
gid_t *childGroup, uid_t *childUser,
int reset_int_quit_handlers,
int flags,
char **failed_doing)
Expand Down Expand Up @@ -144,6 +149,20 @@ runInteractiveProcess (char *const args[],
if ((flags & RUN_PROCESS_IN_NEW_GROUP) != 0) {
setpgid(0, 0);
}

if ( childGroup) {
if ( setgid( *childGroup) != 0) {
// ERROR
childFailed(forkCommunicationFds[1], forkSetgidFailed);
}
}

if ( childUser) {
if ( setuid( *childUser) != 0) {
// ERROR
childFailed(forkCommunicationFds[1], forkSetuidFailed);
}
}

unblockUserSignals();

Expand Down Expand Up @@ -281,6 +300,11 @@ runInteractiveProcess (char *const args[],
case forkExecFailed:
*failed_doing = "runInteractiveProcess: exec";
break;
case forkSetgidFailed:
*failed_doing = "runInteractiveProcess: setgid";
break;
case forkSetuidFailed:
*failed_doing = "runInteractiveProcess: setuid";
default:
*failed_doing = "runInteractiveProcess: unknown";
break;
Expand Down
2 changes: 2 additions & 0 deletions include/runProcess.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ extern ProcHandle runInteractiveProcess( char *const args[],
int *pfdStdInput,
int *pfdStdOutput,
int *pfdStdError,
gid_t *childGroup,
uid_t *childUser,
int reset_int_quit_handlers,
int flags,
char **failed_doing);
Expand Down