*** Bug 484079 has been marked as a duplicate of this bug. ***

I have this problem on Fedora 40 too.

KDE Plasma: 6.0.3
KRDC: krdc-24.02.1-1.fc40

And I really miss Extra Options, since it allow me to enable Dynamic Resolution (/dynamic-resolution).
Also, it would be nice if the GUI has an option for Dynamic Resolution.

Meanwhile, I switch to KRDC 23.08 from Flathub.

Clipboard has been fixed in current development version (see BUG 484666), filesystem sharing is still missing.

*** Bug 484607 has been marked as a duplicate of this bug. ***

As of 24.05, folder sharing still doesn't work.

(In reply to Ninjoe from comment #5)
> As of 24.05, folder sharing still doesn't work.

This is a bug in RdpSession::start().

Actually the code is wrong in a few ways:
1. freerdp_client_add_device_channel() expects the number of strings passed, which should be >= 2 for the "drive" channel. Passing 1 will cause the function to abort (see https://github.com/FreeRDP/FreeRDP/blob/5b14b7cbdd36414f1838047f21502654bd32ebb1/client/common/cmdline.c#L539)
2. strdup is used to copy the string "drive", but the pointer returned is never freed, causing a memory leak (see https://en.cppreference.com/w/c/experimental/dynamic/strdup)
3. The pointer returned by m_preferences->shareMedia().toLocal8Bit().data() is no longer valid by the time it's passed to freerdp_client_add_device_channel, since it references a temporary object that no longer exists at this point

I would suggest the following implementation which I have just tested and that works for me:

    if (!m_preferences->shareMedia().isEmpty()) {
        QByteArray name = "drive";
        QByteArray value = m_preferences->shareMedia().toLocal8Bit();

        char *params[2] = { name.data(), value.data() };
        freerdp_client_add_device_channel(settings, 2, params);
    }

(In reply to Marcel Hasler from comment #6)
> This is a bug in RdpSession::start().

I confirm your  patch fixes the issue. A small variant has been submitted here: https://invent.kde.org/network/krdc/-/merge_requests/114

> 3. The pointer returned by m_preferences->shareMedia().toLocal8Bit().data() 
> is no longer valid by the time it's passed to
> freerdp_client_add_device_channel, since it references a temporary object
> that no longer exists at this point

FreeRDP internally calls _strdup on that parameter in https://github.com/FreeRDP/FreeRDP/blob/stable-2.0/client/common/cmdline.c#L185 , so it's safe to use

Fix has been merged and backported; will be available in 24.08.3, expected release on november 7

(In reply to Fabio from comment #7)
> (In reply to Marcel Hasler from comment #6)
> > This is a bug in RdpSession::start().
> 
> I confirm your  patch fixes the issue. A small variant has been submitted
> here: https://invent.kde.org/network/krdc/-/merge_requests/114
> 
> > 3. The pointer returned by m_preferences->shareMedia().toLocal8Bit().data() 
> > is no longer valid by the time it's passed to
> > freerdp_client_add_device_channel, since it references a temporary object
> > that no longer exists at this point
> 
> FreeRDP internally calls _strdup on that parameter in
> https://github.com/FreeRDP/FreeRDP/blob/stable-2.0/client/common/cmdline.
> c#L185 , so it's safe to use

I don't think this is really true. I actually wrote a small test out of curiosity and found that the data indeed remains accessible, even if the QByteArray has been destroyed at that point (I would have expected a segfault). Apparently Qt doesn't immediately free the memory when a QByteArray goes out of scope but keeps it around for reuse. But here's a simple example to show that you cannot rely on the data remaining valid:

#include <QString>
#include <iostream>

auto main() -> int
{
    char* data1 = nullptr;

    {
        QString test1 = "Test 1";
        data1 = test1.toLocal8Bit().data();
    }

    QString test2 = "Test 2";
    char* data2 = test2.toLocal8Bit().data();
    Q_UNUSED(data2);

    std::cout << data1 << std::endl;

    return 0;
}

(In reply to Marcel Hasler from comment #9)
> But here's a simple example to show that you cannot rely on the data remaining valid:

I know that when the local function variable goes out of scope it's not safe to use anymore.
FreeRDP creates an internal duplicate of the "path" string using _strdup():

> if ((!isPath && !isSpecial) || !(drive->Path = _strdup(path)))

From that point FreeRDP will use its internal copy of the string stored in drive->Path.
So, even if our variable gets deleted, it doesn't matter.

(In reply to Fabio from comment #10)
> (In reply to Marcel Hasler from comment #9)
> > But here's a simple example to show that you cannot rely on the data remaining valid:
> 
> I know that when the local function variable goes out of scope it's not safe
> to use anymore.
> FreeRDP creates an internal duplicate of the "path" string using _strdup():
> 
> > if ((!isPath && !isSpecial) || !(drive->Path = _strdup(path)))
> 
> From that point FreeRDP will use its internal copy of the string stored in
> drive->Path.
> So, even if our variable gets deleted, it doesn't matter.

The point I was trying to make is that by the time freerdp actually calls _strdup, both temporary QByteArray instances have already been destroyed and the pointers are therefore no longer valid. It just *happens* to work because the data hasn't yet been freed or reused/modified in the meantime, but that doesn't mean the code is correct or that it will continue to work in the future. The data just *happens* to still be available, but really you are relying on undocumented behavior here. In fact, the Qt folks could modify their internal implementation at any point (even from a patch version to the next), causing your code to crash or misbehave in other ways.

As I see it, the solution is really simple here, just create two local variables and avoid any potential trouble in the future.

(In reply to Marcel Hasler from comment #11)
> The point I was trying to make is that by the time freerdp actually calls
> _strdup, both temporary QByteArray instances have already been destroyed and
> the pointers are therefore no longer valid.

It's true in your code example in https://bugs.kde.org/show_bug.cgi?id=482137#c9 because of the curly braces specifically added to declare data1 inside a different scope.
It's not true in the code used in the fix for krdc: https://invent.kde.org/network/krdc/-/merge_requests/114/diffs

(In reply to Fabio from comment #12)
> (In reply to Marcel Hasler from comment #11)
> > The point I was trying to make is that by the time freerdp actually calls
> > _strdup, both temporary QByteArray instances have already been destroyed and
> > the pointers are therefore no longer valid.
> 
> It's true in your code example in
> https://bugs.kde.org/show_bug.cgi?id=482137#c9 because of the curly braces
> specifically added to declare data1 inside a different scope.
> It's not true in the code used in the fix for krdc:
> https://invent.kde.org/network/krdc/-/merge_requests/114/diffs

Yes it is. Remove the curly braces and see for yourself.

Created attachment 175249 [details]
Test case 1

Created attachment 175250 [details]
Test case 2

Created attachment 175251 [details]
Test case 3

I can see you are hard to convince, but I'll give it one more shot.

Please have a look at the test cases I attached. Read them, understand them, compile them and run them. If that doesn't convince you, then I guess nothing will. But know that even though your code may happen to "work" in this case, that's by chance, not by design. If you continue doing stuff like that, your luck *will* run out eventually.

Looking at the third test case I finally got your point.
Thank you for your patience 

Fixed in https://invent.kde.org/network/krdc/-/merge_requests/116/diffs

(In reply to Fabio from comment #18)
> Looking at the third test case I finally got your point.
> Thank you for your patience 
> 
> Fixed in https://invent.kde.org/network/krdc/-/merge_requests/116/diffs

Thanks! Just glad I managed to get my point across eventually :-)

(In reply to Marcel Hasler from comment #17)
> I can see you are hard to convince, but I'll give it one more shot.
> 
> Please have a look at the test cases I attached. Read them, understand them,
> compile them and run them. If that doesn't convince you, then I guess
> nothing will. But know that even though your code may happen to "work" in
> this case, that's by chance, not by design. If you continue doing stuff like
> that, your luck *will* run out eventually.

I guess that warning came too late, the same thing is in fact already being done in other places. As a test I printed the proxy and gateway settings with qDebug() just before the call to freerdp_connect(). As expected the output is complete garbage and after the message that it cannot connect, krdc crashes.

According to the sources it looks like freerdp actually takes ownership of all settings strings and frees them on exit, so using strdup (or _strdup from <winpr/string.h> or std::strdup once available) would be the correct solution here.

qstrdup is already used for ServerHostname, Username and Password. I would, however, recommend using C strdup, because qstrdup uses new[] to allocate the string which should be deallocated with delete[] accordingly, whereas freerdp naturally uses free() instead. So using qstrdup to copy the strings would once again be UB.

(In reply to Marcel Hasler from comment #20)
> I guess that warning came too late, the same thing is in fact already being
> done in other places. As a test I printed the proxy and gateway settings
> with qDebug() just before the call to freerdp_connect(). As expected the
> output is complete garbage and after the message that it cannot connect,
> krdc crashes.
> 
> According to the sources it looks like freerdp actually takes ownership of
> all settings strings and frees them on exit, so using strdup (or _strdup
> from <winpr/string.h> or std::strdup once available) would be the correct
> solution here.

Thank you for checking the rest of the code.
A MR is already queued that adds support for freerdp3 and also changes the way all the settings are set, using freerdp_settings_set_string() that internally calls update_string_copy_(), so that freerdp creates its own copy of the char* when they still exists.
I guess that should fix the other issues you found, see eg.:
https://invent.kde.org/network/krdc/-/merge_requests/113/diffs?commit_id=e1c9965f52d0aa7e2e0d9d02f9b4417914ea8748#22035f78003a0693934d972e85db265b9deaa2f4_474_574


> 
> qstrdup is already used for ServerHostname, Username and Password. I would,
> however, recommend using C strdup, because qstrdup uses new[] to allocate
> the string which should be deallocated with delete[] accordingly, whereas
> freerdp naturally uses free() instead. So using qstrdup to copy the strings
> would once again be UB.

Thanks, will integrate this in the MR