Skip to content

Conversation

PatriceJiang
Copy link
Contributor

prevent free _bytes when src and dest are identical.

@PatriceJiang PatriceJiang changed the title cocos2d::Data: fix bug *this = *this cocos2d::Data: prevent use after free. Sep 11, 2018
@minggo
Copy link
Contributor

minggo commented Sep 12, 2018

@PatriceJiang what problem does this PR fix? Could you please add a test case for it?

@PatriceJiang
Copy link
Contributor Author

it is not really a bug.
just protect rare cases like this

Data d;
d.copy(d.getBytes(), len); //expect to be no side effect

@minggo
Copy link
Contributor

minggo commented Sep 13, 2018

I think it is more reasonable to protect in clear(), like this

void Data::clear()
{
    if (_bytes)
        free(_bytes);

    _bytes = nullptr;
    _size = 0;
}

{
//do not free if bytes == _bytes
_bytes = nullptr;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why need to set to nullptr here? It seems it has not effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when size == 0 and bytes == _bytes, _bytes should be pointed to nullptr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if size == 0, then it means doesn't copy anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this is a better way

@Dimon4eg
Copy link
Contributor

Now in copy memory leak possible:

Data d;
int buff[3] = {1, 2, 3};
d.copy(buff, 3); // memory allocated
d.copy(d.getBytes(), 0); //leak here

@PatriceJiang
Copy link
Contributor Author

  1. prevent copy when size is negative
  2. no copy when bytes == _bytes
ssize_t Data::copy(const unsigned char* bytes, const ssize_t size)
{
    CCASSERT(size >= 0, "copy size should be non-negative");

    if (size <= 0) return 0;

    if (bytes != _bytes)
    {
        clear();
    }
    else
    {
        _size = size;
        return _size;
    }
    
    if (size > 0)
    {
        _bytes = (unsigned char*)malloc(sizeof(unsigned char) * _size);
        memcpy(_bytes, bytes, _size);
    }
    _size = size;
    return _size;
}

@Dimon4eg what do you think?

@Dimon4eg
Copy link
Contributor

Much better. Just don't like return in the middle of the function.

Copy link
Contributor

@Dimon4eg Dimon4eg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect!

@minggo minggo added this to the 3.18 milestone Sep 14, 2018
@minggo minggo merged commit d9b2e05 into cocos2d:v3 Sep 14, 2018
stevetranby added a commit to stevetranby/cocos2d-x that referenced this pull request Oct 1, 2018
…v3-merging

* commit 'a59519a4ef567df7aabac77646d49baa4709ff01': (65 commits)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#19082)
  Fix several bugs with button's title (cocos2d#19073)
  [android] fix crash when point JNIEnv is a nullptr in getStringUTFCharsJNI func. (cocos2d#19079)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#19074)
  refactor cmake on use libs (cocos2d#19054)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#19068)
  [linux] CCFileUtils::isFileExistInternal should reject a folder (cocos2d#19062)
  Correct last delta time (cocos2d#18898)
  Add missing <functional> header which the standard requires and will be necessary to use std::function in VS2019. (cocos2d#18989)
  fix wrong parameter (cocos2d#19060)
  DrawNode add isolate flag (cocos2d#19056)
  Android: CCFileUtils listFiles impl (cocos2d#19044)
  CameraBackgroundDepthBrush should handle opengl recreate event. (cocos2d#19037)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#19052)
  shorten the android build path, reduce long path error (cocos2d#19045)
  CCFileUtils: improve thread safety (use single recursive_mutex) (cocos2d#19046)
  remove assert (cocos2d#19047)
  [ci skip][AUTO]: updating luabinding & jsbinding & cocos_file.json automatically (cocos2d#19048)
  Android: Adds supporting edge screens (cocos2d#19043)
  cocos2d::Data: prevent use after free. (cocos2d#19034)
  ...

# Conflicts:
#	build/cocos2d_libs.xcodeproj/project.pbxproj
#	cocos/3d/Android.mk
#	cocos/Android.mk
#	cocos/deprecated/CCDeprecated.h
#	cocos/editor-support/cocostudio/Android.mk
#	cocos/editor-support/spine/Android.mk
#	cocos/platform/android/libcocos2dx-with-controller/build.gradle
#	cocos/platform/android/libcocos2dx/build.gradle
#	cocos/ui/UIAbstractCheckButton.cpp
#	cocos/ui/UIButton.cpp
#	cocos/ui/UIEditBox/UIEditBox.h
#	cocos/ui/UIEditBox/UIEditBoxImpl-common.h
#	extensions/Android.mk
#	tests/cpp-tests/Classes/NewAudioEngineTest/NewAudioEngineTest.cpp
#	tests/cpp-tests/Classes/NewAudioEngineTest/NewAudioEngineTest.h
@PatriceJiang PatriceJiang deleted the cocos2d_Data_fix_assign_to_self branch December 25, 2018 04:02
huangwei1024 pushed a commit to huangwei1024/cocos2d-x that referenced this pull request Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants