95599 - nsClipboard free()'s memory the wrong way...

Status: RESOLVED WONTFIX

(Core :: XUL, defect, P3)

Description

[Roland Mainz]

GTK+ 2001-08-11-08-trunk build on Solaris 7 SPARC build with Sun Workshop 6
Update 2 FCS.

A purify session (Rational Purify) shows that nsClipboard code uses free()
instead of g_free() which may cause heap corruption:

-- snip --
      FUM: Freeing unallocated memory (14 times)
      This is occurring while in:
            free           [rtlib.o]
            *unknown func* [pc=0xfeca9950]
            void nsMemoryImpl::Free(void*) [nsMemoryImpl.cpp]
            void nsMemory::Free(void*) [nsMemoryImpl.cpp]
            unsigned
nsClipboard::HasDataMatchingFlavors(nsISupportsArray*,int,int*)
[nsClipboard.cpp]
            unsigned nsPlaintextEditor::CanPaste(int,int*)
[nsPlaintextDataTransfer.cpp]
            unsigned nsPasteCommand::IsCommandEnabled(const
nsAString&,nsISupports*,int*) [nsEditorCommands.cpp]
            unsigned nsControllerCommandManager::IsCommandEnabled(const
nsAString&,nsISupports*,int*) [nsControllerCommandManager.cpp]
            unsigned nsEditorController::IsCommandEnabled(const nsAString&,int*)
[nsEditorController.cpp]
            XPTC_InvokeByIndex [libxpcom.so]
            int
XPCWrappedNative::CallMethod(XPCCallContext&,XPCWrappedNative::CallMode)
[xpcwrappednative.cpp]
            int XPC_WN_CallMethod(JSContext*,JSObject*,unsigned,long*,long*)
[xpcwrappednativejsops.cpp]
      Attempting to free block at 0xe86d10 in the heap, not obtained from
malloc.
      Address 0xe86d10 is 8 bytes into a malloc'd block at 0xe86d08 of 33 bytes.
      This block was allocated from:
            malloc         [rtlib.o]
            g_malloc       [gmem.c]
            void nsClipboard::SelectionReceiver(_GtkWidget*,_GtkSelectionData*)
[nsClipboard.cpp]
            void
nsClipboard::SelectionReceivedCB(_GtkWidget*,_GtkSelectionData*,unsigned)
[nsClipboard.cpp]
            gtk_marshal_NONE__POINTER_INT [gtkmarshal.c]
            gtk_handlers_run [gtksignal.c]
            gtk_signal_real_emit [gtksignal.c]
            gtk_signal_emit_by_name [gtksignal.c]
            gtk_selection_retrieval_report [gtkselection.c]
            gtk_selection_notify [gtkselection.c]
            gtk_marshal_BOOL__POINTER [gtkmarshal.c]
            gtk_signal_real_emit [gtksignal.c]
-- snip --

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That's not the cause of the FMM you're seeing, though, since they both go down
to malloc and free, at least for now...

Comment 2

[Roland Mainz]

AFAIK the pointer from g_malloc() is not the same as from the underlying
malloc() call, right ? If yes --> heap corruption...

Comment 3

[Mike Pinkerton (not reading bugmail)]

linux clipboard to dr, but i don't quite know how we are running into this 
problem. Isn't nsMemory supposed to free us from all this mumbo-jumbo?

Comment 4

[Dan Rosen]

No pun intended, I hope...

I really have no idea when it comes to things like this. It does look like we're
not going through nsMemory to allocate the chunk that is later freed. That's the
only clue I have here. OTOH, it looks like nsClipboard::SelectionReceiver
(http://lxr.mozilla.org/mozilla/source/widget/src/gtk/nsClipboard.cpp#519) uses
gtk native calls (g_new(), g_free() and less importantly g_print()) all over the
place.

I could try converting all the gtk memory calls in gtk/nsClipboard to use
nsMemory, but I have no idea whether that would fix anything. What do you think,
dbaron?

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't think that's the problem.  IIRC, g_malloc is just a thin wrapper around
malloc.  What the purify log is saying is that we're calling free on a pointer
that's 8 bytes into a block created by a malloc call that was asked to allocate
33 bytes.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, there is certain debugging code where g_malloc isn't just a thin wrapper. 
So if that code is enabled, then that could explain this.  g_malloc is:

gpointer
g_malloc (gulong size)
{
  gpointer p;
  
  
#if defined(ENABLE_MEM_PROFILE) || defined(ENABLE_MEM_CHECK)
  gulong *t;
#endif /* ENABLE_MEM_PROFILE || ENABLE_MEM_CHECK */
  
  
  if (size == 0)
    return NULL;
  
  
#if defined(ENABLE_MEM_PROFILE) || defined(ENABLE_MEM_CHECK)
  size += SIZEOF_LONG;
#endif /* ENABLE_MEM_PROFILE || ENABLE_MEM_CHECK */
  
#ifdef ENABLE_MEM_CHECK
  size += SIZEOF_LONG;
#endif /* ENABLE_MEM_CHECK */
  
  
  p = (gpointer) malloc (size);
  if (!p)
    g_error ("could not allocate %ld bytes", size);
  
  
#ifdef ENABLE_MEM_CHECK
  size -= SIZEOF_LONG;
  
  t = p;
  p = ((guchar*) p + SIZEOF_LONG);
  *t = 0;
#endif /* ENABLE_MEM_CHECK */
  
#if defined(ENABLE_MEM_PROFILE) || defined(ENABLE_MEM_CHECK)
  size -= SIZEOF_LONG;
  
  t = p;
  p = ((guchar*) p + SIZEOF_LONG);
  *t = size;
  
#ifdef ENABLE_MEM_PROFILE
  g_mutex_lock (mem_profile_lock);
#  ifdef ENABLE_MEM_PROFILE_EXCLUDES_MEM_CHUNKS
  if(!IS_IN_MEM_CHUNK_ROUTINE()) {
#  endif
    if (size <= MEM_PROFILE_TABLE_SIZE - 1)
      allocations[size-1] += 1;
    else
      allocations[MEM_PROFILE_TABLE_SIZE - 1] += 1;
    allocated_mem += size;
#  ifdef ENABLE_MEM_PROFILE_EXCLUDES_MEM_CHUNKS
  }
#  endif
  g_mutex_unlock (mem_profile_lock);
#endif /* ENABLE_MEM_PROFILE */
#endif /* ENABLE_MEM_PROFILE || ENABLE_MEM_CHECK */
  
  
  return p;
}

Comment 7

[Dan Rosen]

So that's interesting... Perhaps nsMemory's call to free makes an invalid
assumption about how much we g_malloc'ed. We do seem to use them interchangeably
in that file. I'll put together a patch for y'all to test.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, the nsClipboard code is a real mess as far as allocator consistency for
mSelectionData.data goes.

Comment 9

[Roland Mainz]

Please correct me - this g_malloc() code "destroys" the gurantee that allocated
memory is _always_ (on SPARC) 16byte aligned. That may work with gcc (which
includes a hack to access unaligned memory by sacrifying performance) but not
with Sun Workshop ... ;-(

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

That's true, but only if you have those debugging features enabled.  But it's
not the cause of this bug.

Comment 11

[Dan Rosen]

Attachment: [patch] converts g_new and g_free to nsMemory::Alloc and Free

Comment 12

[Dan Rosen]

This patch is a simple conversion. There's one call to g_free left in the static
__gtk_selection_target_list_remove callback which seemed like it was in the
midst of some heavy-duty glib code, and I didn't want to mess with it.

Could somebody try purifying with this patch? I have no idea whether this will
actually do anything to solve the problem :-/

Comment 13

[John Morrison]

May God have mercy on us all. The 212 bug spam-o-rama is Now!

Comment 14

[Dan Rosen]

Huh?

David or Roland, can you please test this patch in Purify? Otherwise I'll have
to push this bug out to 0.9.5. Thanks.

Comment 15

[John Morrison]

Bug needs a new owner. I pick pavlov since nsClipboard.cpp is mostly his 
historically.

Unsetting milestone. If Roland or David have an idea what the right fix should 
be, and whether the attached patch is what we want then please jump in.

Comment 16

[Roland Mainz]

I am still getting FUM (Freeing unallocated memory) with this patch:
-- snip --
      FUM: Freeing unallocated memory (8 times)
      This is occurring while in:
            free           [rtlib.o]
            PR_Free        [prmem.c:82]
            void nsMemoryImpl::Free(void*) [nsMemoryImpl.cpp]
            void nsMemory::Free(void*) [nsMemoryImpl.cpp]
            void nsClipboard::SelectionReceiver(_GtkWidget*,_GtkSelectionData*)
[nsClipboard.cpp]
            void
nsClipboard::SelectionReceivedCB(_GtkWidget*,_GtkSelectionData*,unsigned)
[nsClipboard.cpp]
            gtk_marshal_NONE__POINTER_INT [gtkmarshal.c]
            gtk_handlers_run [gtksignal.c]
            gtk_signal_real_emit [gtksignal.c]
            gtk_signal_emit_by_name [gtksignal.c]
            gtk_selection_retrieval_report [gtkselection.c]
            gtk_selection_notify [gtkselection.c]
      Attempting to free block at 0x12552c0 in the heap, not obtained from
malloc.
      Address 0x12552c0 is 8 bytes into a malloc'd block at 0x12552b8 of 13
bytes.
      This block was allocated from:
            malloc         [rtlib.o]
            g_malloc       [gmem.c]
            g_strdup       [gstrfuncs.c]
            gdk_atom_name  [gdkproperty.c]
            void nsClipboard::SelectionReceiver(_GtkWidget*,_GtkSelectionData*)
[nsClipboard.cpp]
            void
nsClipboard::SelectionReceivedCB(_GtkWidget*,_GtkSelectionData*,unsigned)
[nsClipboard.cpp]
            gtk_marshal_NONE__POINTER_INT [gtkmarshal.c]
            gtk_handlers_run [gtksignal.c]
            gtk_signal_real_emit [gtksignal.c]
            gtk_signal_emit_by_name [gtksignal.c]
            gtk_selection_retrieval_report [gtkselection.c]
            gtk_selection_notify [gtkselection.c]
      FUM: Freeing unallocated memory (28 times)
      This is occurring while in:
            free           [rtlib.o]
            PR_Free        [prmem.c:82]
            void nsMemoryImpl::Free(void*) [nsMemoryImpl.cpp]
            void nsMemory::Free(void*) [nsMemoryImpl.cpp]
            unsigned
nsClipboard::HasDataMatchingFlavors(nsISupportsArray*,int,int*)
[nsClipboard.cpp]
            unsigned nsPlaintextEditor::CanPaste(int,int*)
[nsPlaintextDataTransfer.cpp]
            unsigned nsPasteCommand::IsCommandEnabled(const
nsAString&,nsISupports*,int*) [nsEditorCommands.cpp]
            unsigned nsControllerCommandManager::IsCommandEnabled(const
nsAString&,nsISupports*,int*) [nsControllerCommandManager.cpp]
            unsigned nsEditorController::IsCommandEnabled(const nsAString&,int*)
[nsEditorController.cpp]
            XPTC_InvokeByIndex [libxpcom.so]
            int
XPCWrappedNative::CallMethod(XPCCallContext&,XPCWrappedNative::CallMode)
[xpcwrappednative.cpp]
            int XPC_WN_CallMethod(JSContext*,JSObject*,unsigned,long*,long*)
[xpcwrappednativejsops.cpp]
      Attempting to free block at 0xdf3920 in the heap, not obtained from
malloc.
      Address 0xdf3920 is 8 bytes into a malloc'd block at 0xdf3918 of 16 bytes.
      This block was allocated from:
            malloc         [rtlib.o]
            g_malloc       [gmem.c]
            g_strdup       [gstrfuncs.c]
            gdk_atom_name  [gdkproperty.c]
            unsigned
nsClipboard::HasDataMatchingFlavors(nsISupportsArray*,int,int*)
[nsClipboard.cpp]
            unsigned nsPlaintextEditor::CanPaste(int,int*)
[nsPlaintextDataTransfer.cpp]
            unsigned nsPasteCommand::IsCommandEnabled(const
nsAString&,nsISupports*,int*) [nsEditorCommands.cpp]
            unsigned nsControllerCommandManager::IsCommandEnabled(const
nsAString&,nsISupports*,int*) [nsControllerCommandManager.cpp]
            unsigned nsEditorController::IsCommandEnabled(const nsAString&,int*)
[nsEditorController.cpp]
            XPTC_InvokeByIndex [libxpcom.so]
            int
XPCWrappedNative::CallMethod(XPCCallContext&,XPCWrappedNative::CallMode)
[xpcwrappednative.cpp]
            int XPC_WN_CallMethod(JSContext*,JSObject*,unsigned,long*,long*)
[xpcwrappednativejsops.cpp]
-- snip --

Finally... without running Purify I get sporadic heap corruption crashes after
heavy use of Clipboard... I think this is a 0.9.5 BLOCKER...

Comment 17

[Stuart Parmenter]

.

Comment 18

[Brant Gurganus]

By the definitions on <http://bugzilla.mozilla.org/bug_status.html#severity> and
<http://bugzilla.mozilla.org/enter_bug.cgi?format=guided>, crashing and dataloss
bugs are of critical or possibly higher severity.  Only changing open bugs to
minimize unnecessary spam.  Keywords to trigger this would be crash, topcrash,
topcrash+, zt4newcrash, dataloss.

Comment 19

[Mats Palmgren (inactive)]

Does this bug occur in a recent trunk build?

FWIW, I don't think the patch is the right thing to do.
The API documentation says the value should be deallocated using g_free()
http://www-eleves-isia.cma.fr/documentation/GtkDoc/gdk/gdk-properties-and-atoms.html#GDK-ATOM-NAME

Comment 20

[Mats Palmgren (inactive)]

Gtk1/xlib widget code has been removed on trunk.

-> WONTFIX