Bad access to memory using strcat

3
votes

I'm using linux.

I have a function called like:

PlayBackgroundIntroMusic((char *)"IntroMusic");

The functions is:

void SoundManager::
PlayBackgroundIntroMusic( char * musicFile)
{
        // Concatenate extension for each platform
        strcat (musicFile,audioExtension);
        CCLOG("musicFile: %c" musicFile);
  SimpleAudioEngine::sharedEngine()->playBackgroundMusic(std::string(CCFileUtils::fullPathFromRelativePath(musicFile)).c_str(), false);
}

But i having a bad access to memory on line:

strcat (musicFile,audioExtension);

audioExtension is declarated:

#include 
using std::string;
#include 
using std::cout; using std::cerr; using std::endl;

/**
 * Declare sound extension for each platform
 * Android = ogg
 * iOS = caf
 * WIN32 = mp3
 */

#if (CC_TARGET_PLATFORM == CC_PLATFORM_WIN32)
        static const char * audioExtension = ".wav";
#elif (CC_TARGET_PLATFORM == CC_PLATFORM_IOS)
        static const char * audioExtension = ".caf";
#elif (CC_TARGET_PLATFORM == CC_PLATFORM_ANDROID)
        static const char * audioExtension = ".ogg";
#endif

So, i expected to have:

IntroMusic.caf on iOS
IntroMusic.ogg on Android

What's is happening?

NOTE: I have tried to:

 char * musicFileWithExtension = strcat (musicFile,audioExtension);

But it didn't work anyway.

musicFile is not a constant. I don't want to declarate a tempchar[80] to avoid overflow if the name of the file is too long like Example cc reference

Thanks in advance.

3
c++stdstring-concatenationcocos2d-x
Your compiler tried to tell you something. Then to shut it up, you added a cast. Maybe you should have listened to it. – Benjamin Lindley
Does it work without the cast? Exercise: find out why not. – ta.speot.is
With out the (char *) cast, a warning is send, conversion decreated, so i do the cast. What is going on? Is a bad use to cast the string? – vgonisanz
You trying to catenate something without storage for it. AFAIR an ordinary string literal has type “array of n const char” - from C++ std. – Sergei Nikulov

3 Answers

5
votes

String literals, such as "IntroMusic" are of type const char[N] which is implicitly convertible to const char *. Through a mistake in language design, it is also convertible to char*, but that conversion is rightfully deprecated in C++, hence the warning. You need to use an array (dynamically or statically allocated), not a string literal.

Or better yet, use std::string.

2
votes

First of all, "IntroMusic" is const.

Removing const-ness from a const value and modify it, is undefined behaviour. Anything might happen, you are lucky to get the crash right away.

Furthermore, the memory allocated for "IntroMusic" is exactly 10 bytes for the characters plus a delimiting \0, so in total 11 bytes. Period. Now, besides you trying to force a modification of a const value, you even write to unallocated memory (at least, not allocated by you for the purpose of writing to it): You simply try to write your platform-dependent file extension to the memory after "IntroMusic".

You are responsible to provide a sufficiently large buffer for your operation.

Simple solution (since you tagged the question c++, not c: use std::string

1
votes

Look at the strcat documentation. It is adding destination string to the source string. In your case source string is "musicFile", so it should not be constant and should have enough length.

if function is called like this:

PlayBackgroundIntroMusic((char *)"IntroMusic");

then musicFile == "IntroMusic" is constant and cannot be overwrited.