Bug 4886 - clean up subprocess pipe handling on Windows
Summary: clean up subprocess pipe handling on Windows
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: Client (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.2.0
Assignee: Pierre Ossman
URL:
Keywords: hean01_tester, prosaic
Depends on:
Blocks:
 
Reported: 2013-11-05 14:05 CET by Pierre Ossman
Modified: 2014-04-14 10:36 CEST (History)
1 user (show)

See Also:
Acceptance Criteria:


Attachments
backtrace (13.21 KB, text/plain)
2014-04-10 12:31 CEST, Henrik Andersson
Details
logfile (6.58 KB, text/plain)
2014-04-10 12:43 CEST, Henrik Andersson
Details

Description Pierre Ossman cendio 2013-11-05 14:05:28 CET
We are currently a bit sloppy when it comes to handling the pipe handles for subprocesses on Windows. AppVerifier even makes the client crash because it has tests that make sure you use file handles in a safe way.

The primary problem is that WinPopenProcess closes the pipes, even though ServiceProcess is still using them.

It might be sufficient to throw a couple of Fl::check() in WinProcess::KillProcess() to make sure ServiceProcess notices that the pipe is closed.

Related, we also need to fix WinPopenProcess so that it doesn't close the pipes until _after_ the process has been terminated.
Comment 1 Pierre Ossman cendio 2013-11-05 14:06:36 CET
(see bug 4815 and bug 4569 for the AppVerifier crashes)
Comment 2 Pierre Ossman cendio 2013-12-19 13:33:52 CET
All done. Tester should run tlclient -d5 and look for PeekNamedPipe errors, and that logging seems complete. Probably also best to test with AppVerifier for the previously seen crashes.
Comment 3 Henrik Andersson cendio 2014-04-10 12:31:39 CEST
Created attachment 527 [details]
backtrace

Got a new backtrace in tlclient, right after hitting connect button.
Comment 4 Henrik Andersson cendio 2014-04-10 12:43:27 CEST
Created attachment 529 [details]
logfile
Comment 5 Pierre Ossman cendio 2014-04-10 13:59:36 CEST
Whatever this is, it is not this bug. AppVerifier makes PeekNamedPipe() return ERROR_NOACCESS randomly on perfectly valid file handles. Adding bug 5087 instead.
Comment 6 Henrik Andersson cendio 2014-04-14 10:36:51 CEST
Tested using client build 4323, works as expected. No errors in log file when trying to provoke killing sub processes to client.

Note You need to log in before you can comment on or make changes to this bug.