tags:

views:

313

answers:

6

I have this C-styled piece of initialization code:

const char * const vlc_args[] =
{
    "-I", "dummy",
    "--ignore-config",
    "--extraintf=logger",
    "--verbose=2"
    "--plugin-path=/usr/lib/vlc"
};
//tricky calculation of the char space used
libvlc_new(sizeof(vlc_args)/sizeof(vlc_args[0]), vlc_args, &exc);

Since I need to make the --plugin-path parameter dynamic, I can't use a static array anymore. So I came up with a C++ alternative:

std::string pluginpath = "test";
libvlc_exception_t exc;

std::vector<std::string> args;
args.push_back("-I");
args.push_back("dummy");
args.push_back("--ignore-config");
args.push_back("--extraintf=logger");
args.push_back("--verbose=2");
args.push_back("--ipv4");
args.push_back("--plugin-path=" + pluginpath);

std::string combinedString;
for (size_t idx = 0; idx < args.size(); ++idx)
{
    combinedString.append(args[idx]);
    combinedString.resize(combinedString.size() + 1);
    combinedString[combinedString.size() - 1] = 0;
}
combinedString.resize(combinedString.size() + 1);
combinedString[combinedString.size() - 1] = 0;

size_t size = combinedString.size();
const char * data = combinedString.c_str();
libvlc_new(size, &data, &exc); // => error occurs here (not at end of scope or anything)

But this results in a segmentation fault. So there must be an error in my code, which I can't seem to find.. Can anyone spot it?

Solved!
Thanks to Joseph Grahn and Jason Orendorff. My idea on the memory layout of the C-style array was wrong. I thought all data was organized as a big sequential block. In reality it's a list of pointers to the first character of each individual string.

This code works:

std::vector<const char*> charArgs;
for (size_t idx = 0; idx < args.size(); ++idx)
{
    charArgs.push_back(&(args[idx][0]));
}
mVLCInstance = libvlc_new(charArgs.size(),
                          &charArgs[0],
                          &mVLCException);
A: 

The problem is due to the use of c_str() and storing the pointer.

See http://stackoverflow.com/questions/1374468/c-stringstream-string-and-char-conversion-confusion

EDIT Forget what I said... it was late... see the comments :)

Tristram Gräbener
This situation is different from using a stringstream incorrectly (ss.str().c_str()) because in my string object is an l-value.
StackedCrooked
The pointer returned by `combinedString.c_str()` is valid until `combinedString` is modified or destroyed.
sth
+4  A: 

You are appending all arguments into a single string, then you pass a pointer to the const char * string to libvlc_new as if it were an array of char *.

(I'm not sure this is the problem, but it seems a bit strange.)

Josef Grahn
How does is it different from an array of char*? Isn't the memory layout in both cases the same?
StackedCrooked
No, "const char * const vlc_args[]" is an array of pointers. There aren't any pointers in the single string.
Paul Tomblin
I don't know what libvlc expects, but for program arguments as in main(), you are supposed to be able to do: argv[3] to get a const char * to the forth argument, for example. In your case that would yield a wild pointer somewhere on the stack (above where variable data resides).
Josef Grahn
Dang, you're right.
StackedCrooked
+4  A: 

I think Josef Grahn is right: the API wants an actual array of pointers.

If you don't need to add arguments programmatically, you can just go back to using an array:

std::string pluginpath = "test";
std::string pluginpath_arg = "--plugin-path=" + pluginpath;
const char *args[] = {
    "-I", dummy, "--ignore-config", ..., pluginpath_arg.c_str()
    };

libvlc_exception_t exc;
libvlc_new(sizeof(args) / sizeof(args[0]), args, &exc);

EDIT: There might also be a problem with using c_str() here. This is true if VLC keeps the pointer and uses it again later; I can't tell if that's the case from the docs.

Jason Orendorff
I wonder if the sizeof(0) is good, I'd use sizeof(char*).
epatel
or sizeof args[0].
Alex
I remember reading that the Google code guidelines dictate using sizeof(args[0]) instead of sizeof(char) so it doesn't need to be changed (or forgotten) in case the data type changes (wchar_t for example).
StackedCrooked
@StackedCrooked thats good too...
epatel
Thanks Jason, your answer was the one that got me thinking in the right way.
StackedCrooked
epatel: Good eye, I meant `sizeof(args[0])`.
Jason Orendorff
+1  A: 

Regarding the segmentation violation

No solution, as there will probably be more problems

You are sending only 1 string in. (not sure if it is allowed by libvlc_new) So the first parameter should be set to 1, ie size = 1. I believe this will solve the segmentation problem. But I doubt libvlc_new can be called with just one line of multiple parameters.

In the original code sizeof(vlc_args)/sizeof(vlc_args[0]) will have the number of parameters as entries in the vector. In your example equal 6.

Your code

size_t size = combinedString.size(); // a long string, size >> 1
const char * data = combinedString.c_str(); // data is a pointer to the string

libvlc_new(size, &data, &exc);

// size should be 1 because data is like an array with only one string. 
// &data is the adress to the "array" so to speak. If size > 1 the function
// will try to read pass the only string available in this "array"

I think Jason Orendorff has a good solution to fix it all...

epatel
+1  A: 

The library call expects a pointer to an array of const char* (that is several pointers), but you pass it a single pointer. That more characters got appended to the end of that string doesn't matter.

To dynamically build an array of the required pointers you could use another vector:

// store c_str() pointers in vector
std::vector<const char*> combined;
for (size_t idx = 0; idx < args.size(); ++idx) {
    combined.push_back(args[idx].c_str());
}

// pass pointer to begin of array (it is guaranteed that the contents of a vector
// are stored in a continuous memory area)
libvlc_new(combined.size(), &(combined[0]), &exc);

Jason Orendorff's remark is also valid here: This will not work if libvlc_new stores the passed pointer internally for later use.

sth
+1  A: 

Have you tried:

libvlc_new(size, data, &exc);

instead of

libvlc_new(size, &data, &exc);

It seems you use the null bytes to make the string act like an array of characters, but then you pass a pointer to the char* "array" instead of just the array.

FranticPedantic