Bug 5222 - RDPDR channel sometimes stops working when a smart card is inserted / removed on Windows 2012 R2 server
Summary: RDPDR channel sometimes stops working when a smart card is inserted / removed...
Status: CLOSED FIXED
Alias: None
Product: ThinLinc
Classification: Unclassified
Component: | rdesktop (deprecated) (show other bugs)
Version: trunk
Hardware: PC Unknown
: P2 Normal
Target Milestone: 4.3.0
Assignee: Henrik Andersson
URL:
Keywords: prosaic, samuel_tester
Depends on:
Blocks: 4912
  Show dependency treegraph
 
Reported: 2014-08-20 08:40 CEST by Henrik Andersson
Modified: 2014-10-07 16:52 CEST (History)
1 user (show)

See Also:
Acceptance Criteria:


Attachments

Description Henrik Andersson cendio 2014-08-20 08:40:53 CEST
Works perfectly fine when doing so against Windows 2012 Server.
Comment 1 Henrik Andersson cendio 2014-09-04 08:55:53 CEST
There is a bug in pcsclite in call chain SCardGetStatusChange()->WaitForPcscdEvent() where a timeout value of 0 will block the call instead of beeing a poll.

On rdesktop side I can make a workaround to change all timeouts from 0 to 1ms for polling functionality, this however will introduce a hang of a few ms. but thats better then hanging forever.
Comment 2 Henrik Andersson cendio 2014-09-05 08:03:58 CEST
Another bug with pcsclite discovered:

SCardGetStatusChange() locks a context which results with other operations using the same context like SCardListReaders() will hang until SCardGetStatusChange() finished.

A workaround on rdesktop side is to create a new internal context for each dispatch of SCardGetStatusChange() which seems to work as expected.
Comment 3 Henrik Andersson cendio 2014-09-05 14:00:00 CEST
Identified a bug a potential memory leak in handling of IRP_MJ_DEVICE_CONTROL requests. Each dispatched request is put into list using add_async_iorequest()
which is processed using select and built around the use of a fd, serial/file io. Smartcard doesn't use a fd and therefor it's never removed form this list.

Simple fix, don't put smartcard request into async_iorequest(), scard.c have internal handling of request and their response.
Comment 4 Henrik Andersson cendio 2014-09-09 09:41:58 CEST
(In reply to comment #3)
> Identified a bug a potential memory leak in handling of IRP_MJ_DEVICE_CONTROL
> requests. Each dispatched request is put into list using add_async_iorequest()
> which is processed using select and built around the use of a fd, serial/file
> io. Smartcard doesn't use a fd and therefor it's never removed form this list.
> 
> Simple fix, don't put smartcard request into async_iorequest(), scard.c have
> internal handling of request and their response.

This is not an issue, scard.c:thread_wrapper(), the fns->device_control() implementation adds 0xc0000000 to status code which is used to skip adding pending request to list.
Comment 5 Henrik Andersson cendio 2014-09-11 13:24:29 CEST
The problem is now pinpointed to:

During log on to a session, the RDPDR channel is initialize so that smartcard can be used. After the log on there is a reinitialize of RDPDR channel that also implies that all pending iorequest is abdonend. If rdesktop have pending responses to iorequest received on initial RDPDR channel which will be sent to the server on the reinitialized RDPDR channel, the server will shutdown the RDPDR channel and no further packets will be sent/received over RDPDR.
Comment 6 Henrik Andersson cendio 2014-09-11 13:25:22 CEST
(In reply to comment #1)
> There is a bug in pcsclite in call chain
> SCardGetStatusChange()->WaitForPcscdEvent() where a timeout value of 0 will
> block the call instead of beeing a poll.
> 
> On rdesktop side I can make a workaround to change all timeouts from 0 to 1ms
> for polling functionality, this however will introduce a hang of a few ms. but
> thats better then hanging forever.

Workaround in upstream commit 1835.
Comment 7 Henrik Andersson cendio 2014-09-11 13:49:39 CEST
(In reply to comment #0)
> Works perfectly fine when doing so against Windows 2012 Server.

Testcase:

The issue appeared once you inserted/removed the card from the reader. The issue is 50% reproducible with following apporach:

Connect with CredSSP + Kerberos, local drive, smartcard redirection and provide username/password on commandline for rdesktop.

1. *Reconnect* to a session with smartcard insterted
2. Open the local drive redirection in Explorer so you can
   see the files...
3. Remove card from reader
4. Refresh explorer window and you will be prompted with a
   message '\\tsclient\xxx is not accessible...'
Comment 8 Henrik Andersson cendio 2014-09-11 14:02:32 CEST
(In reply to comment #5)
> The problem is now pinpointed to:
> 
> During log on to a session, the RDPDR channel is initialize so that smartcard
> can be used. After the log on there is a reinitialize of RDPDR channel that
> also implies that all pending iorequest is abdonend. If rdesktop have pending
> responses to iorequest received on initial RDPDR channel which will be sent to
> the server on the reinitialized RDPDR channel, the server will shutdown the
> RDPDR channel and no further packets will be sent/received over RDPDR.

Workaround for the issue is commited upstream in r1836.

The workaround is to prevent sending iorequest responses to abandoned iorequests on a reinitialized RDPDR channel by tagging iorequest to a specific epoch.
Comment 9 Henrik Andersson cendio 2014-09-11 14:09:43 CEST
(In reply to comment #8)
> (In reply to comment #5)
> > The problem is now pinpointed to:
> > 
> > During log on to a session, the RDPDR channel is initialize so that smartcard
> > can be used. After the log on there is a reinitialize of RDPDR channel that
> > also implies that all pending iorequest is abdonend. If rdesktop have pending
> > responses to iorequest received on initial RDPDR channel which will be sent to
> > the server on the reinitialized RDPDR channel, the server will shutdown the
> > RDPDR channel and no further packets will be sent/received over RDPDR.
> 
> Workaround for the issue is commited upstream in r1836.
> 
> The workaround is to prevent sending iorequest responses to abandoned
> iorequests on a reinitialized RDPDR channel by tagging iorequest to a specific
> epoch.

Created bug #5252 for introducing resource leak in the workaround.
Comment 10 Henrik Andersson cendio 2014-09-11 14:20:12 CEST
(In reply to comment #8)
> (In reply to comment #5)
> > The problem is now pinpointed to:
> > 
> > During log on to a session, the RDPDR channel is initialize so that smartcard
> > can be used. After the log on there is a reinitialize of RDPDR channel that
> > also implies that all pending iorequest is abdonend. If rdesktop have pending
> > responses to iorequest received on initial RDPDR channel which will be sent to
> > the server on the reinitialized RDPDR channel, the server will shutdown the
> > RDPDR channel and no further packets will be sent/received over RDPDR.
> 
> Workaround for the issue is commited upstream in r1836.
> 
> The workaround is to prevent sending iorequest responses to abandoned
> iorequests on a reinitialized RDPDR channel by tagging iorequest to a specific
> epoch.

Brought to ctc in commit 29346.
Comment 11 Henrik Andersson cendio 2014-09-11 15:53:30 CEST
(In reply to comment #1)
> There is a bug in pcsclite in call chain
> SCardGetStatusChange()->WaitForPcscdEvent() where a timeout value of 0 will
> block the call instead of beeing a poll.
> 
> On rdesktop side I can make a workaround to change all timeouts from 0 to 1ms
> for polling functionality, this however will introduce a hang of a few ms. but
> thats better then hanging forever.

Posted this issue to the maling list due to i didn't find any issue tracker for the project.
Comment 12 Samuel Mannehed cendio 2014-09-26 10:50:14 CEST
Was not able to reproduce the issue on Windows 2012 R2 Server. Will verify that nothing is broken after the changes on Windows 2008 R2 server before closing.
Comment 13 Samuel Mannehed cendio 2014-10-07 16:52:06 CEST
(In reply to comment #12)
> Was not able to reproduce the issue on Windows 2012 R2 Server. Will verify that
> nothing is broken after the changes on Windows 2008 R2 server before closing.

Looks good

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