Create a C style char** from a C++ vector

up vote
5
down vote

favorite

I am dealing with some older C style APIs, like Posix execve that take a char**. In the rest of my code, I prefer to use a fairly modern C++ style, so I have vector of std::string. Is this the best way to handle the glue between them?

char ** strlist(std::vector<std::string> &input) 
 char** result = new char*[input.size()];
 result[input.size()] = nullptr;
 for (int i=0; i<input.size(); i++) 
 char *temp = new char[input[i].size()];
 strcpy(temp, input[i].c_str());
 result[i] = temp;
 
 return result;



bool del_strlist(char ** strings, int limit=1024) 
 bool finished = false;
 for (int i=0; i<limit; i++) 
 char *temp = strings[i];
 if (temp == nullptr) 
 finished = true;
 break;
 
 delete temp;
 
 delete strings;
 return !finished;

edited 1 hour ago

Stephen Rauch

3,75051530

asked 2 hours ago

wrosecrans

1262

New contributor

add a commentÂ |Â

up vote
5
down vote

favorite

char ** strlist(std::vector<std::string> &input) 
 char** result = new char*[input.size()];
 result[input.size()] = nullptr;
 for (int i=0; i<input.size(); i++) 
 char *temp = new char[input[i].size()];
 strcpy(temp, input[i].c_str());
 result[i] = temp;
 
 return result;



bool del_strlist(char ** strings, int limit=1024) 
 bool finished = false;
 for (int i=0; i<limit; i++) 
 char *temp = strings[i];
 if (temp == nullptr) 
 finished = true;
 break;
 
 delete temp;
 
 delete strings;
 return !finished;

edited 1 hour ago

Stephen Rauch

3,75051530

asked 2 hours ago

wrosecrans

1262

New contributor

add a commentÂ |Â

up vote
5
down vote

favorite

char ** strlist(std::vector<std::string> &input) 
 char** result = new char*[input.size()];
 result[input.size()] = nullptr;
 for (int i=0; i<input.size(); i++) 
 char *temp = new char[input[i].size()];
 strcpy(temp, input[i].c_str());
 result[i] = temp;
 
 return result;



bool del_strlist(char ** strings, int limit=1024) 
 bool finished = false;
 for (int i=0; i<limit; i++) 
 char *temp = strings[i];
 if (temp == nullptr) 
 finished = true;
 break;
 
 delete temp;
 
 delete strings;
 return !finished;

edited 1 hour ago

Stephen Rauch

3,75051530

asked 2 hours ago

wrosecrans

1262

New contributor

char ** strlist(std::vector<std::string> &input) 
 char** result = new char*[input.size()];
 result[input.size()] = nullptr;
 for (int i=0; i<input.size(); i++) 
 char *temp = new char[input[i].size()];
 strcpy(temp, input[i].c_str());
 result[i] = temp;
 
 return result;



bool del_strlist(char ** strings, int limit=1024) 
 bool finished = false;
 for (int i=0; i<limit; i++) 
 char *temp = strings[i];
 if (temp == nullptr) 
 finished = true;
 break;
 
 delete temp;
 
 delete strings;
 return !finished;

c++ c posix

edited 1 hour ago

Stephen Rauch

3,75051530

asked 2 hours ago

wrosecrans

1262

New contributor

edited 1 hour ago

Stephen Rauch

3,75051530

asked 2 hours ago

wrosecrans

1262

New contributor

edited 1 hour ago

Stephen Rauch

3,75051530

edited 1 hour ago

Stephen Rauch

3,75051530

edited 1 hour ago

Stephen Rauch

3,75051530

asked 2 hours ago

wrosecrans

1262

New contributor

asked 2 hours ago

wrosecrans

1262

asked 2 hours ago

wrosecrans

1262

New contributor

wrosecrans is a new contributor to this site. Take care in asking for clarification, commenting, and answering.
Check out our Code of Conduct.

add a commentÂ |Â

2 Answers
2

active

oldest

votes

up vote
3
down vote

I can see a few potential problems here:

Since you allocated a char* array of input.size() elements, result[input.size()] is out of bounds.

Similarly, std::string's size() is the number of characters - it doesn't include the trailing needed for C-style strings. So every strcpy here risks doing a buffer overflow (risks, because it is possible for C++ std::strings to contain a null in the middle, terminating the strcpy mid way).

You have set a limit on the number of elements of strings you delete, but then delete strings irrespective of whether that limit was breached. This risks a memory leak if the limit was exceeded.

answered 56 mins ago

muru

454212

add a commentÂ |Â

up vote
0
down vote

Well, aside from what @muru mentioned, I'd like to provide slightly different take on this:

`std::vector<char*>`

I believe std::vector<char*> is suitable for this purpose and better than char**. I believe the code is intended to be called after forking, because otherwise, from my understanding, nothing is gonna work anyway. I also believe that the code waits for the child process to exit, as the code has deletion function.

With a little bit more usage of standard library, one can build something like this:

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;

It doesn't have many advantages compared to your solution, though. Deletion function thus becomes:

void deallocate_cstrings(const std::vector<char*>& source) 
 for (auto&& elem: source) 
 delete elem;

Real improvement

Now, the above is very small improvement over the original. The real advantage of the above code would come when the deletion function would be wrapped within one small class or struct:

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

It's just that I'm too paranoid about memory leaks. Unfortunately std::unique_ptr<> doesn't guarantee that pointer is the only non-static member when deleter is empty.

Demo

A small demo on Wandbox:

#include <vector>
#include <string>
#include <algorithm>

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;


#include <iostream>

template <typename T>
std::ostream& operator<<(std::ostream& os, const std::vector<T>& v) 
 if (v.empty()) 
 return os; 
 
 os << v.front();

 for (std::size_t i = 1; i < v.size(); ++i) 
 os << ' ' << v[i];
 
 return os;


int main() 
 std::vector<std::string> words = "What", "a", "beautiful", "world";
 std::cout << words << 'n';

 auto moved_words = owned_cstringsmove_strings(words);
 moved_words.cstring_array.push_back(nullptr); //null terminate for exec style
 //use moved_words.data() to get char**
 std::cout << moved_words.cstring_array << 'n';

answered 4 mins ago

Incomputable

6,15121349

add a commentÂ |Â

Your Answer

StackExchange.ifUsing("editor", function ()
return StackExchange.using("mathjaxEditing", function ()
StackExchange.MarkdownEditor.creationCallbacks.add(function (editor, postfix)
StackExchange.mathjaxEditing.prepareWmdForMathJax(editor, postfix, [["\$", "\$"]]);
);
);
, "mathjax-editing");

StackExchange.ifUsing("editor", function ()
StackExchange.using("externalEditor", function ()
StackExchange.using("snippets", function ()
StackExchange.snippets.init();
);
);
, "code-snippets");

StackExchange.ready(function()
var channelOptions =
tags: "".split(" "),
id: "196"
;
initTagRenderer("".split(" "), "".split(" "), channelOptions);

StackExchange.using("externalEditor", function()
// Have to fire editor after snippets, if snippets enabled
if (StackExchange.settings.snippets.snippetsEnabled)
StackExchange.using("snippets", function()
createEditor();
);

else
createEditor();

);

function createEditor()
StackExchange.prepareEditor(
heartbeatType: 'answer',
convertImagesToLinks: false,
noModals: false,
showLowRepImageUploadWarning: true,
reputationToPostImages: null,
bindNavPrevention: true,
postfix: "",
onDemand: true,
discardSelector: ".discard-answer"
,immediatelyShowMarkdownHelp:true
);

);

wrosecrans is a new contributor. Be nice, and check out our Code of Conduct.

draft saved

draft discarded

StackExchange.ready(
function ()
StackExchange.openid.initPostLogin('.new-post-login', 'https%3a%2f%2fcodereview.stackexchange.com%2fquestions%2f205269%2fcreate-a-c-style-char-from-a-c-vectorstring%23new-answer', 'question_page');

);

Post as a guest

Name

2 Answers
2

active

oldest

votes

2 Answers
2

active

oldest

votes

up vote
3
down vote

I can see a few potential problems here:

Since you allocated a char* array of input.size() elements, result[input.size()] is out of bounds.

Similarly, std::string's size() is the number of characters - it doesn't include the trailing needed for C-style strings. So every strcpy here risks doing a buffer overflow (risks, because it is possible for C++ std::strings to contain a null in the middle, terminating the strcpy mid way).

You have set a limit on the number of elements of strings you delete, but then delete strings irrespective of whether that limit was breached. This risks a memory leak if the limit was exceeded.

answered 56 mins ago

muru

454212

add a commentÂ |Â

up vote
3
down vote

I can see a few potential problems here:

Since you allocated a char* array of input.size() elements, result[input.size()] is out of bounds.

Similarly, std::string's size() is the number of characters - it doesn't include the trailing needed for C-style strings. So every strcpy here risks doing a buffer overflow (risks, because it is possible for C++ std::strings to contain a null in the middle, terminating the strcpy mid way).

You have set a limit on the number of elements of strings you delete, but then delete strings irrespective of whether that limit was breached. This risks a memory leak if the limit was exceeded.

answered 56 mins ago

muru

454212

add a commentÂ |Â

up vote
3
down vote

I can see a few potential problems here:

Since you allocated a char* array of input.size() elements, result[input.size()] is out of bounds.

Similarly, std::string's size() is the number of characters - it doesn't include the trailing needed for C-style strings. So every strcpy here risks doing a buffer overflow (risks, because it is possible for C++ std::strings to contain a null in the middle, terminating the strcpy mid way).

You have set a limit on the number of elements of strings you delete, but then delete strings irrespective of whether that limit was breached. This risks a memory leak if the limit was exceeded.

answered 56 mins ago

muru

454212

I can see a few potential problems here:

Since you allocated a char* array of input.size() elements, result[input.size()] is out of bounds.

Similarly, std::string's size() is the number of characters - it doesn't include the trailing needed for C-style strings. So every strcpy here risks doing a buffer overflow (risks, because it is possible for C++ std::strings to contain a null in the middle, terminating the strcpy mid way).

You have set a limit on the number of elements of strings you delete, but then delete strings irrespective of whether that limit was breached. This risks a memory leak if the limit was exceeded.

answered 56 mins ago

muru

454212

answered 56 mins ago

muru

454212

answered 56 mins ago

muru

454212

answered 56 mins ago

muru

454212

add a commentÂ |Â

up vote
0
down vote

Well, aside from what @muru mentioned, I'd like to provide slightly different take on this:

`std::vector<char*>`

With a little bit more usage of standard library, one can build something like this:

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;

It doesn't have many advantages compared to your solution, though. Deletion function thus becomes:

void deallocate_cstrings(const std::vector<char*>& source) 
 for (auto&& elem: source) 
 delete elem;

Real improvement

Now, the above is very small improvement over the original. The real advantage of the above code would come when the deletion function would be wrapped within one small class or struct:

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

It's just that I'm too paranoid about memory leaks. Unfortunately std::unique_ptr<> doesn't guarantee that pointer is the only non-static member when deleter is empty.

Demo

A small demo on Wandbox:

#include <vector>
#include <string>
#include <algorithm>

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;


#include <iostream>

template <typename T>
std::ostream& operator<<(std::ostream& os, const std::vector<T>& v) 
 if (v.empty()) 
 return os; 
 
 os << v.front();

 for (std::size_t i = 1; i < v.size(); ++i) 
 os << ' ' << v[i];
 
 return os;


int main() 
 std::vector<std::string> words = "What", "a", "beautiful", "world";
 std::cout << words << 'n';

 auto moved_words = owned_cstringsmove_strings(words);
 moved_words.cstring_array.push_back(nullptr); //null terminate for exec style
 //use moved_words.data() to get char**
 std::cout << moved_words.cstring_array << 'n';

answered 4 mins ago

Incomputable

6,15121349

add a commentÂ |Â

up vote
0
down vote

Well, aside from what @muru mentioned, I'd like to provide slightly different take on this:

`std::vector<char*>`

With a little bit more usage of standard library, one can build something like this:

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;

It doesn't have many advantages compared to your solution, though. Deletion function thus becomes:

void deallocate_cstrings(const std::vector<char*>& source) 
 for (auto&& elem: source) 
 delete elem;

Real improvement

Now, the above is very small improvement over the original. The real advantage of the above code would come when the deletion function would be wrapped within one small class or struct:

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

It's just that I'm too paranoid about memory leaks. Unfortunately std::unique_ptr<> doesn't guarantee that pointer is the only non-static member when deleter is empty.

Demo

A small demo on Wandbox:

#include <vector>
#include <string>
#include <algorithm>

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;


#include <iostream>

template <typename T>
std::ostream& operator<<(std::ostream& os, const std::vector<T>& v) 
 if (v.empty()) 
 return os; 
 
 os << v.front();

 for (std::size_t i = 1; i < v.size(); ++i) 
 os << ' ' << v[i];
 
 return os;


int main() 
 std::vector<std::string> words = "What", "a", "beautiful", "world";
 std::cout << words << 'n';

 auto moved_words = owned_cstringsmove_strings(words);
 moved_words.cstring_array.push_back(nullptr); //null terminate for exec style
 //use moved_words.data() to get char**
 std::cout << moved_words.cstring_array << 'n';

answered 4 mins ago

Incomputable

6,15121349

add a commentÂ |Â

up vote
0
down vote

Well, aside from what @muru mentioned, I'd like to provide slightly different take on this:

`std::vector<char*>`

With a little bit more usage of standard library, one can build something like this:

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;

It doesn't have many advantages compared to your solution, though. Deletion function thus becomes:

void deallocate_cstrings(const std::vector<char*>& source) 
 for (auto&& elem: source) 
 delete elem;

Real improvement

Now, the above is very small improvement over the original. The real advantage of the above code would come when the deletion function would be wrapped within one small class or struct:

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

It's just that I'm too paranoid about memory leaks. Unfortunately std::unique_ptr<> doesn't guarantee that pointer is the only non-static member when deleter is empty.

Demo

A small demo on Wandbox:

#include <vector>
#include <string>
#include <algorithm>

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;


#include <iostream>

template <typename T>
std::ostream& operator<<(std::ostream& os, const std::vector<T>& v) 
 if (v.empty()) 
 return os; 
 
 os << v.front();

 for (std::size_t i = 1; i < v.size(); ++i) 
 os << ' ' << v[i];
 
 return os;


int main() 
 std::vector<std::string> words = "What", "a", "beautiful", "world";
 std::cout << words << 'n';

 auto moved_words = owned_cstringsmove_strings(words);
 moved_words.cstring_array.push_back(nullptr); //null terminate for exec style
 //use moved_words.data() to get char**
 std::cout << moved_words.cstring_array << 'n';

answered 4 mins ago

Incomputable

6,15121349

Well, aside from what @muru mentioned, I'd like to provide slightly different take on this:

`std::vector<char*>`

With a little bit more usage of standard library, one can build something like this:

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;

It doesn't have many advantages compared to your solution, though. Deletion function thus becomes:

void deallocate_cstrings(const std::vector<char*>& source) 
 for (auto&& elem: source) 
 delete elem;

Real improvement

Now, the above is very small improvement over the original. The real advantage of the above code would come when the deletion function would be wrapped within one small class or struct:

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

It's just that I'm too paranoid about memory leaks. Unfortunately std::unique_ptr<> doesn't guarantee that pointer is the only non-static member when deleter is empty.

Demo

A small demo on Wandbox:

#include <vector>
#include <string>
#include <algorithm>

struct owned_cstrings 
 std::vector<char*> cstring_array;

 ~owned_cstrings() 
 for (auto&& elem: cstring_array) 
 delete elem;
 
 
;

std::vector<char*> move_strings(const std::vector<std::string>& source) 
 std::vector<char*> destination(source.size());
 std::transform(source.begin(), source.end(), destination.begin(), (const auto& elem_str) 
 char* buffer = new char[elem_str.size() + 1];
 //guaranteed by C++11 and later to have 0 at the end
 std::copy(elem_str.begin(), ++elem_str.end(), buffer); 
 return buffer;
 );

 return destination;


#include <iostream>

template <typename T>
std::ostream& operator<<(std::ostream& os, const std::vector<T>& v) 
 if (v.empty()) 
 return os; 
 
 os << v.front();

 for (std::size_t i = 1; i < v.size(); ++i) 
 os << ' ' << v[i];
 
 return os;


int main() 
 std::vector<std::string> words = "What", "a", "beautiful", "world";
 std::cout << words << 'n';

 auto moved_words = owned_cstringsmove_strings(words);
 moved_words.cstring_array.push_back(nullptr); //null terminate for exec style
 //use moved_words.data() to get char**
 std::cout << moved_words.cstring_array << 'n';

answered 4 mins ago

Incomputable

6,15121349

answered 4 mins ago

Incomputable

6,15121349

answered 4 mins ago

Incomputable

6,15121349

answered 4 mins ago

Incomputable

6,15121349

add a commentÂ |Â

wrosecrans is a new contributor. Be nice, and check out our Code of Conduct.

draft saved

draft discarded

wrosecrans is a new contributor. Be nice, and check out our Code of Conduct.

draft saved

draft discarded

Post as a guest

Name

Search This Blog

Iyfjky