Opened 8 months ago

Last modified 7 months ago

#15914 accepted defect

Inconsistency between wxTextDataObject::GetData{Size,Here}() in wxOSX

Reported by: kristianduske Owned by: csomor
Priority: normal Milestone:
Component: wxOSX-Cocoa Version: dev-latest
Keywords: wxDataObject Cc:
Blocked By: Blocking:
Patch: no

Description

In dnd_osx.cpp, the function wxDropTarget::GetData() uses a char buffer to obtain the contents of the wxDataObject associated with the current drop source. This buffer is too small (by one char). Its size is determined by calling wxDataObject::GetDataSize(wxDataFormat format), which does not account for the fact that wxDataObject::GetDataHere(...) will append a null char if the data is a string. To fix this, the buffer size must be increased by one in line 132 of dnd_osx.cpp:

                        char *d = new char[size+1];

I was not able to reproduce this crash with the dnd sample, but it leads to heap corruption in my app. Enabling malloc debugging in XCode lead me to the actual defect.

Alternatively, wxTextDataObject::GetDataSize(...) could be changed to account for the null char.

Change History (7)

comment:1 Changed 8 months ago by csomor

  • Owner set to csomor
  • Status changed from new to accepted

comment:2 Changed 8 months ago by vadz

I think GetDataSize() should include the trailing NUL. MSW already does it and I think the others should too.

comment:3 Changed 7 months ago by vadz

  • Keywords wxDataObject added
  • Milestone 3.0.1 deleted
  • Summary changed from Crash in wxDropTarget::GetData() on OS X to Inconsistency between wxTextDataObject::GetData{Size,Here}() in wxOSX
  • Version changed from 3.0.0 to dev-latest

Unfortunately this code is not very easy to understand, the assumptions of various functions are not clear and there are no tests for it (which is understandable because testing dnd non-interactively is not obvious but still doesn't help) so I'm too afraid of changing wxTextDataObject itself. I am still pretty sure that it is the code there that should be fixed however as it's just wrong for GetDataHere() to use more bytes than GetDataSize() returns.

But for now I'll just make the buffer bigger to avoid overrunning it.

comment:4 Changed 7 months ago by VZ

(In [76050]) Fix off by 1 error in buffer size in wxOSX wxDropTarget code.

The size of the buffer used for the data currently needs to include an extra
byte for the trailing NUL. This is wrong, as it means that GetDataSize() and
GetDataHere() behaviour is not consistent, but at least avoid overrunning the
buffer for now.

Also use wxCharBuffer instead of raw char array to make the code safer (both
because it releases the memory automatically and because it also adds an extra
byte for the trailing NUL automatically as well, making such bugs impossible).

See #15914.

comment:5 Changed 7 months ago by VZ

(In [76056]) Fix off by 1 error in buffer size in wxOSX wxDropTarget code.

The size of the buffer used for the data currently needs to include an extra
byte for the trailing NUL. This is wrong, as it means that GetDataSize() and
GetDataHere() behaviour is not consistent, but at least avoid overrunning the
buffer for now.

Also use wxCharBuffer instead of raw char array to make the code safer (both
because it releases the memory automatically and because it also adds an extra
byte for the trailing NUL automatically as well, making such bugs impossible).

See #15914.

comment:6 Changed 7 months ago by csomor

OSX in many places just uses TEXT data with no additional zero at the end as the size is retrieved with other means, I was comparing the other parts at the time for GetDataSize and I leaned towards the implementation in dobjcmn which returns the size without the trailing zero. The inconsistency between GetDataHere and GetDataSize also exists there.

comment:7 Changed 7 months ago by vadz

I think it exists for wxGTK and should be fixed there too. AFAIR it's ok in wxMSW.

The reason I'm against it is that the code working with wxDataObject is generic, it doesn't know whether it works with the text or the bitmap, so it can't add 1 (or 2 or 4 depending on wchar_t size) extra byte just for text, unless you do it unconditionally which is plainly ugly. So either it should be included in the GetDataSize() return value or GetDataHere() shouldn't NUL-terminate the text, but this is rather dangerous.

Note: See TracTickets for help on using tickets.