FreeRDP/FreeRDP — pull request #220 by celsius (2011-11-24 15:52:03+00:00)
Title: fix three double frees in redirection.c/settings.c and a memory leak in connection.c

Hi

For some time now I've been running into some segfault-like issue when closing session in a redirected environment.

I used gdb and valgrind to pinpoint (and more or less solve) the problem (as of git d778564) :

rdp_free() - rdp.c: (lines 840 and 848)

> settings_free(rdp->settings);
> redirection_free(rdp->redirection);

settings_free() - settings.c (lines 168-171) :

> xfree(settings->hostname);
> xfree(settings->username);
> xfree(settings->password);
> xfree(settings->domain);

redirection_free() - redirection.c (lines 190-192 and 195)

> freerdp_string_free(&redirection->username);//--> problem: double free()
> freerdp_string_free(&redirection->domain);//--> problem
> freerdp_string_free(&redirection->password);//--> no problem ! (not copied in rdp_client_redirect() )
> freerdp_string_free(&redirection->targetNetAddress);//--> problem

settings->hostname, settings->username, settings->domain have respectively the same address than redirection->targetNetAddress->ascii, redirection->username->ascii and redirection->domain->ascii (cf. connection.c:rdp_client_redirect())

So there is a double free on each one of these (and eventually a crash).

Also, I think all that might be linked with another potential "problem" in connection.c, line 136, 138, 141 and 144 :

> settings->hostname = redirection->targetNetAddress.ascii;
> settings->hostname = redirection->targetNetBiosName.ascii;
> settings->username = redirection->username.ascii;
> settings->domain = redirection->domain.ascii;

I think the three "settings->..." must be freed before assignment ?

Also, this part of the code makes it not clear to me whether settings_free() or redirection_free() should be used to free these three.

Besides redirection->hostname.unicode, redirection->username.unicode, redirection->domain.unicode must be freed in redirection_free()

so I propose to set the redirection->....ascii to NULL so that freerdp_string_free() doesn't double-frees these anymore but still frees redirection->....unicode.

Hope it helps,

Alexis