215587 - memory leak in nsClipboard::GetData

Status: RESOLVED FIXED

(Core Graveyard :: GFX: Xlib, defect)

Description

[William Bardwell]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.0) Gecko/20020529
Build Identifier: recent 1.5

nsClipboard::GetData mallocs space for the final param passed to
XGetWindowPropery and then frees what was put in that param.
This is wrong, that param should be the address of a pointer that
is NULL, XGetWindowProperty overwrites the param with a pointer
to space that X allocated itself.  You should then XFree the
pointer.  (So in the current code, the malloced space is leaked, and
the X allocated space is freed with the wrong routine (but it might
get away with that if XFree is the same as free.)

Reproducible: Always

Steps to Reproduce:
1.
2.
3.




Will attach simple patch

Comment 1

[William Bardwell]

Attachment: patch to ./widget/src/xlib/nsClipboard.cpp that fixes leak.

Comment 2

[William Bardwell]

Attachment: Better patch widget/src/xlib/nsClipboard.cpp

Old code looks like it could sometimes return random data,
because it doesn't always fill in "data".  I made a second
patch that just returns an empty string in those cases,
and fixes the memory leak.

Comment 3

[timeless]

thanks

Comment 4

[Akkana Peck]

Comment on attachment 129471 [details] [diff] [review]
Better patch widget/src/xlib/nsClipboard.cpp

Yes, this is better.  Thanks!

Does length end up being set right?  Should we force it to zero if data == 0,
or just assume that strlen already did that for us?

Comment 5

[William Bardwell]

length/bytes will be 0 if XGetWindowProperty is never
called, or if it returns no data.  That code has other
problems, like the fact that it can skip calling
XGetWindowProperty but still try to convert an empty
string, and that you are supposed to keep calling
XGetWindowProperty (updating the offset) until
it has actually read all of the data for the property
(right now it just reads the first 16k).
I think that the clipboard code may need a re-write
in general, there are a lot of clipboard related bugs...
This fix still helps with some basic reliability
issues, if not the correctness ones.

Comment 6

[jag (Peter Annema)]

Comment on attachment 129471 [details] [diff] [review]
Better patch widget/src/xlib/nsClipboard.cpp

>--- ./widget/src/xlib/nsClipboard.cpp	Fri Mar 14 20:04:05 2003
>+++ nsClipboard.cpp	Fri Aug  8 17:46:53 2003
>@@ -342,7 +342,6 @@
>@@ -375,8 +374,8 @@
> 
>     // Place the data in the transferable
>     PRInt32 length = bytes;
>-    PRUnichar *testing = (PRUnichar *)malloc(strlen((char *)data)*2+1);
>-    nsPrimitiveHelpers::ConvertPlatformPlainTextToUnicode((const char *)data,
>+    PRUnichar *testing = (PRUnichar *)malloc(data?strlen((char *)data)*2+1:3);
>+    nsPrimitiveHelpers::ConvertPlatformPlainTextToUnicode((const char *)data?data:"",
>                                                           length,
>                                                           &testing,
>                                                           &length);

That +1 should really be inside the *2 (which should be *sizeof(PRUnichar)),
the null terminator is also 16 bits. The 3 should be 2:
|(strlen("")+1)*sizeof(PRUnichar)|. 

>@@ -391,7 +390,8 @@
>     aTransferable->SetTransferData("text/unicode",
>                                    genDataWrapper,
>                                    length);
>-    free(data);
>+    if (data)
>+      XFree(data);

Why XFree? It was |malloc|'ed, so |free|.

Comment 7

[William Bardwell]

The XFree is because it was not malloced.
XGetWindowProperty allocates the data. Part of the
fix is to stop mallocing an block and then passing the
address of the pointer to the malloced block to
XGetWindowProperty, which was then overwriting that
pointer with the address of the space that XGetWindowProperty
allocated itself.

Comment 8

[timeless]

Attachment: don't allocate for convert either

Comment 9

[Akkana Peck]

+    const char *constData = "";
+    if (data)
+       data = (const char*) data;

What are the second and third lines for?  Should that third line be constData =?

Comment 10

[timeless]

oops, yeah.

Comment 11

[timeless]

Attachment: don't allocate for convert either

Comment 12

[timeless]

mozilla/widget/src/xlib/nsClipboard.cpp 	1.25
mozilla/widget/src/xlib/nsClipboard.cpp 	1.26
i messed up the commit