Bug 225559 - net/krdc: "xfreerdp" parameter "/network:[...]" not supported anymore in FreeRDP 2.0.0r1?
Summary: net/krdc: "xfreerdp" parameter "/network:[...]" not supported anymore in Free...
Status: Closed FIXED
Alias: None
Product: Ports & Packages
Classification: Unclassified
Component: Individual Port(s) (show other bugs)
Version: Latest
Hardware: Any Any
: --- Affects Only Me
Assignee: Kyle Evans
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-01-30 12:12 UTC by Nils Beyer
Modified: 2018-08-30 17:54 UTC (History)
3 users (show)

See Also:
bugzilla: maintainer-feedback? (kde)


Attachments
Patch (to add to files/) (539 bytes, patch)
2018-04-08 20:52 UTC, Adriaan de Groot
no flags Details | Diff
Fix command-line handling (1.38 KB, patch)
2018-04-09 10:56 UTC, Adriaan de Groot
no flags Details | Diff
svn(1) diff of net/freerdp (20.44 KB, patch)
2018-04-27 03:24 UTC, Kyle Evans
kevans: maintainer-approval+
Details | Diff
Patch timer implementation (284 bytes, patch)
2018-04-27 10:38 UTC, Adriaan de Groot
no flags Details | Diff
svn(1) diff of net/freerdp (23.60 KB, patch)
2018-05-03 03:36 UTC, Kyle Evans
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nils Beyer 2018-01-30 12:12:23 UTC
After upgrading all packages, it seems that "krdc" is no longer able to spawn RDP sessions. Activating all debug flags using "kdebugdialog" reveals that "krdc" executes "xfreerdp" with following options:
-------------------------------------------------------------------------------
-decorations /w:1255 /h:879 /kbd:0x00000407 /u:SECRETUSER /parent-window:4194338 /bpp:16 /audio-mode:2 /drive:media,/media /network:lan /cert-ignore +clipboard /port:3389 /v:SECRETSERVER
-------------------------------------------------------------------------------


Executing "xfreerdp" manually with above mentioned options line does not spawn a RDP session as well and show "xfreerdp" usage output instead. Removing an option one by one reveals that "xfreerdp" does not like the "/network" parameter anymore - at least with that options combination.

Patching "krdc" to exclude that:
-------------------------------------------------------------------------------
--- rdpview.cpp.bak     2018-01-30 13:08:36.375470000 +0100
+++ rdpview.cpp 2018-01-30 12:53:57.551629000 +0100
@@ -310,7 +310,7 @@
             break;
         }
 
-        arguments << "/network:" + performance;
+        // arguments << "/network:" + performance; // nbe: broken in FreeRDP 2.0.0r1
 
         if (m_hostPreferences->console()) {
             arguments << "/admin";
-------------------------------------------------------------------------------


makes it working again...
Comment 1 Adriaan de Groot freebsd_committer freebsd_triage 2018-04-08 20:39:49 UTC
Reassigning to kevans@ as the maintainer of freerdp.

The problem isn't that krdc is calling freerdp with bad parameters, it's that freerdp 2.0.0-rc1 (current in FreeBSD ports) has bad parameter handling. FreeBSD's strtol() sets errno to EINVAL on bad input, which freerdp doesn't expect.
Comment 2 Adriaan de Groot freebsd_committer freebsd_triage 2018-04-08 20:52:54 UTC
Created attachment 192351 [details]
Patch (to add to files/)

Accept strtol() returning 0 and setting errno to EINVAL, when passed the string "lan". This is different from the behavior Linux GLibC displays.
Comment 3 Nils Beyer 2018-04-09 07:04:26 UTC
(In reply to Adriaan de Groot from comment #2)

thanks for the patch. Unfortunately, it does not fix the problem. To reduce the test case, plese try following command line:
-----------------------------------------------------------------------------
xfreerdp /network:lan /port:3389 /v:127.0.0.1
-----------------------------------------------------------------------------

It still throws the usage information unless you remove the "/network:lan" part...
Comment 4 Adriaan de Groot freebsd_committer freebsd_triage 2018-04-09 10:37:37 UTC
It shows the usage information because of *other* bugs in xfreerdp, though. I mean, sure, we can just remove /network from krdc and ignore the setting the user does there, but the problems are in xfreerdp, in strtoul() or in the way errno is set. Well, mostly in the fact that strtol() sets errno in invalid cases, but doesn't *re*set it on success, while Linux GLibC seems to ignore errno entirely.
Comment 5 Adriaan de Groot freebsd_committer freebsd_triage 2018-04-09 10:56:28 UTC
Created attachment 192358 [details]
Fix command-line handling

Fix handling of errno wrt. strtol() and strtoul()
Comment 6 Nils Beyer 2018-04-09 11:07:30 UTC
(In reply to Adriaan de Groot from comment #5)

thanks for the patch - that fixes the simplified test-case with the parameters "/network:lan /port:3389 /v:127.0.0.1".

But the complete command line - that "krdc" wants to spawn - still fails:
-----------------------------------------------------------------------------
# bad
/usr/local/bin/xfreerdp -decorations /w:1255 /h:879 /kbd:0x00000407 /u:SECRETUSER /parent-window:4194338 /bpp:16 /audio-mode:2 /drive:media,/media /network:lan /cert-ignore +clipboard /port:3389 /v:SECRETSERVER
-----------------------------------------------------------------------------


unless you remove the "/network:[...]" portion:
-----------------------------------------------------------------------------
# good
/usr/local/bin/xfreerdp -decorations /w:1255 /h:879 /kbd:0x00000407 /u:SECRETUSER /parent-window:4194338 /bpp:16 /audio-mode:2 /drive:media,/media /cert-ignore +clipboard /port:3389 /v:SECRETSERVER
-----------------------------------------------------------------------------
Comment 7 Adriaan de Groot freebsd_committer freebsd_triage 2018-04-26 19:23:12 UTC
Kyle, there's an -rc2 for FreeRDP out now, and I've filed issue #4597 upstream referring to this PR. Can you chase the parameter-handling some more?
Comment 8 Kyle Evans freebsd_committer freebsd_triage 2018-04-27 01:02:07 UTC
(In reply to Adriaan de Groot from comment #7)

Hi Adriaan,

Thanks for the patch and help diagnosing! I'm taking a poke at this right now -- I got kind of sidetracked in trying to get things cranked out and snuck in for the upcoming release that I've seriously neglected FreeRDP. =(
Comment 9 Kyle Evans freebsd_committer freebsd_triage 2018-04-27 01:37:57 UTC
Hi,

So basically, your assessment is generally right but we'll need to rework the patch... a lot. This might be more of a Coccinelle job, but basically- every call to strtol, strotul, strtold that tries to check errno != 0 after the fact should be setting errno to 0 before-hand, as per POSIX recommendations [1].

Ideally the return value of these functions would always be validated before errno is consulted, but regardless- a POSIX compliant implementation will not set errno to 0 on success, so errno != 0 checks alone to signify an error are bogus without knowing that errno = 0 going into it.

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/strtol.html
Comment 10 Kyle Evans freebsd_committer freebsd_triage 2018-04-27 03:24:54 UTC
Created attachment 192835 [details]
svn(1) diff of net/freerdp

I've created an upstream pull request here [1] after verifying that it fixes Nils' invocation to not throw usage, at least -- I would wager that it works as intended, though.

I've additionally attached here a patch to update to -rc2 w/ my patch that's been submitted in PR 4598 upstream. If you could both test it, then Adriaan may commit it or I'll do so if he approves me to do so. =) Poudriere and portlint are both relatively happy with it.

[1] https://github.com/FreeRDP/FreeRDP/pull/4598
Comment 11 Nils Beyer 2018-04-27 09:49:24 UTC
(In reply to Kyle Evans from comment #10)

thanks for the patch; I no more get a usage prompt, but "xfreerdp" aborts with:
--------------------------------------------------------------------------
[11:48:19:615] [5109:0a016500] [INFO][com.freerdp.client.common.cmdline] - loading channelEx cliprdr
Password: 
[11:48:21:111] [5109:0a016500] [INFO][com.freerdp.gdi] - Local framebuffer format  PIXEL_FORMAT_BGRX32
[11:48:21:111] [5109:0a016500] [INFO][com.freerdp.gdi] - Remote framebuffer format PIXEL_FORMAT_RGB16
[11:48:21:120] [5109:0a016500] [INFO][com.winpr.clipboard] - initialized POSIX local file subsystem
[11:48:21:121] [5109:0a016500] [ERROR][com.winpr.synch.timer] - InitializeWaitableTimer: os specific implementation is missing
--------------------------------------------------------------------------
Comment 12 Adriaan de Groot freebsd_committer freebsd_triage 2018-04-27 10:37:49 UTC
I'm just shooting in the dark here, because of a lack of anything to test with; there are two ways of getting that error message:

 - you hit a code path that wants timerfd
 - you hit a code path that wants POSIX timers

Using timerfd would require extra ports-dependencies (libepoll-shim, it seems) and some CMake work. Using POSIX timers requires claiming that more OSsen than just Linux have them. Try the attached patch along with what Kyle already has; it might help a little.
Comment 13 Adriaan de Groot freebsd_committer freebsd_triage 2018-04-27 10:38:21 UTC
Created attachment 192843 [details]
Patch timer implementation
Comment 14 Nils Beyer 2018-04-30 09:45:38 UTC
(In reply to Adriaan de Groot from comment #12)

did not help, unfortunately; it still complains about missing OS specific implementation. So it seems that it really needs "timerfd"...
Comment 15 Kyle Evans freebsd_committer freebsd_triage 2018-05-01 13:31:59 UTC
(In reply to Nils Beyer from comment #14)

Hi,

I'm drafting up a new update patch that pulls in epoll-shim for the timerfd implementation. Like Adriaan, I have no way to test this part- I'll make sure to confirm that it at least detects timerfd and builds properly before handing it off to you for testing.
Comment 16 Kyle Evans freebsd_committer freebsd_triage 2018-05-03 03:36:14 UTC
Created attachment 193015 [details]
svn(1) diff of net/freerdp

Attached is a new patch against net/freerdp on a clean ports tree - can you give that a shot, please? This version pulls in libepoll-shim on a version of FreeRDP that's been fixed to detect it in our environment. I've personally tested the timing mechanism, so that roadblock *should* be out of the way.

Do note that all but three of the patches in files/ have been deleted here. I've spent the past couple of days upstreaming all of our patches that I can- most of them have been accepted, I've converted the rest to PATCH_FILES mechanism for the time being as I have pull requests opened upstream for them.
Comment 17 Nils Beyer 2018-05-03 11:48:22 UTC
(In reply to Kyle Evans from comment #16)

thank you very much for the patch.

Works flawlessly (apart from sound) here now. Very nice...
Comment 18 Kyle Evans freebsd_committer freebsd_triage 2018-05-03 13:01:32 UTC
(In reply to Nils Beyer from comment #17)

Awesome, good to hear! I have some other things I want to sneak in before I can commit to this update, but hopefully I'll finish with those within the week. =)

RE: Sound- I personally don't use freerdp+sound, but I can take a look after this other stuff. They just committed a major refactoring of sound channel bits and apparently added an FFMPEG backend for some of this stuff.
Comment 19 Ivan 2018-05-09 11:46:49 UTC
I tested it successfully, but looks like patch is outdated now.

abishai@poudriere:/usr/local/poudriere/ports/default/net/freerdp % doas patch -sEC < /home/abishai/rdp.patch
1 out of 5 hunks failed while patching Makefile

Probably, we should commit it already and thanks for solution!
Comment 20 Adriaan de Groot freebsd_committer freebsd_triage 2018-06-04 07:57:06 UTC
(This PR pings kde@ because it's filed against net/krdc, but it's basically blocked on the updates to net/freerdp which were developed in the course of the comments stream here)
Comment 21 Ivan 2018-08-30 17:15:05 UTC
Proposed changes had been upstreamed and port updated. I can confirm that rdp feature works.
Comment 22 Adriaan de Groot freebsd_committer freebsd_triage 2018-08-30 17:54:06 UTC
Closing based on comment #21 and r478200