Opened 17 years ago

Closed 15 years ago

#815 enhancement closed fixed (fixed)

t.internet.process.Process should check for valid arguments

Reported by: marienz Owned by:
Priority: highest Milestone:
Component: core Keywords:
Cc: Jean-Paul Calderone, itamarst, marienz, therve Branch:
Author:

Description


Attachments (4)

process.patch (1.9 KB) - added by marienz 17 years ago.
process_815.py (2.2 KB) - added by therve 15 years ago.
process_815_2.py (2.8 KB) - added by therve 15 years ago.
process_815_3.py (3.6 KB) - added by therve 15 years ago.

Download all attachments as: .zip

Change History (32)

Changed 17 years ago by marienz

Attachment: process.patch added

comment:1 Changed 17 years ago by marienz

If you pass Process arguments that don't make sense you may not find out until
it tries to call os.execvpe. Unfortunately that call happens after forking and
playing interesting games with fd's, so the exception can "vanish". 

The following patch tries to do some sanity checking before forking.
Unfortunately the actual checks execvpe does are in C code, and I can't
duplicate it exactly. So this won't catch every bad thing you can throw at exec,
and it will complain (rarely) about valid things. See the comments for details.

This does cause two new test failures, both in mail.test.test_mail.py,
TestAliasResolution and TestCyclicAlias. But those tests seem to be a bit bogus
(afaik) so I don't think the failures are a bad thing :)

Changed 15 years ago by therve

Attachment: process_815.py added

comment:2 Changed 15 years ago by therve

Component: conch
Keywords: review added
Priority: normalhighest

Attach a patch with small modification from previous patch (use TypeError, add a new check for string arguments, remove check for empty arguments), and a testcase.

comment:3 Changed 15 years ago by therve

Cc: therve added
Component: conchcore

comment:4 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from itamarst to therve

glanced at the patch briefly; one other thing that will definitely cause exec to fail is a string with nul bytes in it. if we are going to do validation, we should catch and reject these too.

Changed 15 years ago by therve

Attachment: process_815_2.py added

comment:5 Changed 15 years ago by therve

Keywords: review added

Added the null bytes test.

comment:6 Changed 15 years ago by Ralph Meijer

therve, it appears to me that you are not going to review this yourself. It might be useful to reassign ownership.

comment:7 Changed 15 years ago by therve

Owner: therve deleted

Ups, thank you.

comment:8 Changed 15 years ago by Jean-Paul Calderone

Committed this patch to spawnprocess-args-815 with some minor edits. I think it's good to merge, but someone else should take a quick look.

comment:9 Changed 15 years ago by Jean-Paul Calderone

Owner: set to Jean-Paul Calderone
Status: newassigned

comment:10 Changed 15 years ago by Jean-Paul Calderone

Owner: Jean-Paul Calderone deleted
Status: assignednew

Oops, I can't review this one.

comment:11 Changed 15 years ago by Jonathan Lange

Owner: set to Jonathan Lange

comment:12 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: changed from Jonathan Lange to Jean-Paul Calderone
Status: newassigned

On IRC Jerub told me that he reviewed this and it was okay to merge.

comment:13 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: assignedclosed

(In [18491]) Merge spawnprocess-args-815

Author: therve, exarkun Reviewer: jerub Fixes #815

Perform typechecking in the POSIX process launching code and raise TypeErrors before forking if the provided arguments cannot result in a successful execvpe() call.

comment:14 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: closedreopened

Re-opened by r18491

Changed 15 years ago by therve

Attachment: process_815_3.py added

comment:15 Changed 15 years ago by therve

Keywords: review added
Owner: changed from Jean-Paul Calderone to therve
Status: reopenednew

Attached a patch moving the check into posixbase, and using it into win32reactor. It seems to be good under Windows.

comment:16 Changed 15 years ago by therve

Owner: therve deleted

comment:17 Changed 15 years ago by Jean-Paul Calderone

Owner: set to Cory Dodt

comment:18 Changed 15 years ago by Jean-Paul Calderone

Owner: changed from Cory Dodt to Jean-Paul Calderone
Status: newassigned

I was just taking another look at this and noticed that spawnProcess lets unicode strings through. This only works if the unicode string can be encoded using the 'ascii' codec, so it's not clear that we should actually be allowing it. At the very least there should be test coverage for this behavior.

comment:19 Changed 15 years ago by Jean-Paul Calderone

Keywords: review removed

I ended up keeping unicode support, but deprecating it. I need to test this more. The Windows buildslaves are offline right now.

comment:20 Changed 15 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

I guess this seems to be working now.

Please review.

comment:21 Changed 15 years ago by Jean-Paul Calderone

Owner: set to Jonathan Lange

comment:22 Changed 15 years ago by Jonathan Lange

Branch is now spawnprocess-args-815-3

comment:23 Changed 15 years ago by Jonathan Lange

Keywords: review removed
Owner: changed from Jonathan Lange to Jean-Paul Calderone

Looks good. I particularly like the way you've defined the utility classes for test_process.

  • _deprecatedUnicodeSupportTest should really have a docstring. Given that it has assertions, it should explain what it's checking for in its docstring.
  • The docstring for _checkProcessArgs should say the env part of the two-tuple is either a dict or None. Or, it should refer to another docstring which explains the return values.

Fix these and merge away.

comment:24 Changed 15 years ago by Jean-Paul Calderone

Status: newassigned

Fixed those. pre-checkin testing found one problem with gtk2 reactor :/ I'm not sure how to best fix it yet. gtk2 sets the default system encoding to 'utf-8' which breaks assumptions made by the tests. I thought the fix for this was to use 'ascii' instead of getdefaultencoding but now I think that's wrong, and it should still use getdefaultencoding but the tests should be different somehow.

comment:25 Changed 15 years ago by Jean-Paul Calderone

Keywords: review added
Owner: changed from Jean-Paul Calderone to Jonathan Lange
Status: assignednew

I've committed some more changes and a few comments which hopefully make it very clear what is supposed to be going on and what is going on. I'll leave them to explain the changes I made. If they're not clear, please let me know and I'll have another go at them.

comment:26 Changed 15 years ago by Jonathan Lange

Keywords: review removed
Owner: changed from Jonathan Lange to Jean-Paul Calderone

Awesome.

Only change required is from "Anything unicode string" to "Any unicode string" in posixbase.py. Then merge away. Thanks for making this all so clear.

comment:27 Changed 15 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [18898]) Merge spawnprocess-args-815-3

Author: therve, exarkun Reviewer: exarkun, jml Fixes #815

Check the argv and environment values before creating a child process with IReactorProcess.spawnProcess and reject values which will not be allowed by the actual process creation API. This causes spawnProcess to raise synchronously, rather than delivering bytes on stderr to the process protocol used.

Reject anything which is neither str nor unicode for both of these things. Also emit a deprecation warning when unicode is received, since this really shouldn't have been allowed at all.

comment:28 Changed 11 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.