3

Pretty self explanatory. Here's the method that's causing the SIGABRT on the 'new vector' line:

vector<string> * Task::arguments() {
    vector<string> *args = new vector<string>(); // CAUSES SIGABRT
    int count = sizeof(_arguments);
    for (int x = 0; x < count; x++) {
        string argument(_arguments[x]);
        args->push_back(argument);
    }
    return args;
}

Note that elsewhere I call that exact line without any issues. Here is the list of includes in the Task class:

#include <vector>
#include <unistd.h>
#include <string>
using namespace std;
#include <sys/types.h>
#include <sys/wait.h>
#include <signal.h>

Any thoughts?

jstm88
  • 3,335
  • 4
  • 38
  • 55
  • 11
    Why are you dynamically allocating the `std::vector`? You should almost certainly be using a local variable and returning it by value. – James McNellis Feb 16 '11 at 17:24
  • 4
    If you really *must* use `using namespace std;`. Put it *after* all include directives. – Benjamin Lindley Feb 16 '11 at 17:30
  • See also http://stackoverflow.com/questions/5058831 – Robert Harvey Feb 20 '11 at 21:40
  • 1
    The main issue is that sbi's tone in his initial comment showed that he didn't care enough to actually find out what was going on before he assumed things. It was my fault for taking that answer seriously, but that doesn't excuse him for posting an arrogant out of context answer. The issue is resolved, however, and I'm willing to forgive and forget; I only urge people to not make assumptions or out of context answers, especially for those with high "reputation" - people take your answers seriously, so don't lead people on a wild goose chase with a response based on a false assumption. – jstm88 Feb 20 '11 at 22:25

4 Answers4

6

There is no real error in this code, although the style suggests that you urgently need a good C++ book.

As @James said in his comment to the question, it is almost certainly wrong to use a dynamically allocated vector object, and it's certainly wrong to not to keep it in a smart pointer.
If you've also done this elsewhere in your code, you very likely have messed up the heap, which is the only case I can think of when new would crash.

Community
  • 1
  • 1
sbi
  • 219,715
  • 46
  • 258
  • 445
  • Check my answer below - if it's wrong to do that then the C++ designers were much more confused than even I thought... – jstm88 Feb 19 '11 at 23:02
  • 1
    @jfm: We won't moderate answers for technical accuracy. You guys will have to hash that out yourselves. – Robert Harvey Feb 20 '11 at 21:19
  • I can never take this for an answer rather a personal assumption of the user. And we are very unfortunate that a person with such a high reputation had posted such an out-of-context reply. I believe, what is written on this post is actually true as a design guideine but isn't intended and shouldn't be appreciated as a solution to this particular problem. – Ayan Sengupta Feb 04 '14 at 23:56
3

Okay, it turns out the problem was NOT with the vector allocation, but rather inside the for() loop. Replacing it with a proper do/while loop fixed it. GDB was simply confused about where the error was and stuck it on the vector line... ugh. Not surprising given the use of the STL though.

Also, to those who say allocating a vector dynamically can mess up the heap - why? This makes no sense; I don't care if I allocate every single object dynamically; doing that should not cause problems. Mixing heap and stack objects has caused me a lot of problems in the past; the only way I can get things to work consistently is using all heap objects.

If someone can explain how stack and heap objects can coexist, though, I'd be happy to hear it, because they've never worked together for me. Objective-C makes perfect sense, while C++'s nonsensical treatment of objects almost always causes problems. Unfortunately we're required to use C++, so it leaves me without a lot of choice...

jstm88
  • 3,335
  • 4
  • 38
  • 55
  • 1) I can see nothing wrong with that `for` loop. Could you elaborate? 2) Nobody said using a dynamic vector will mess up the heap. However, everybody that doing so is stupid. 3) If everything that doesn't make sense to you was indeed nonsensical, this world would be a silly please. Fortunately, that isn't so. 3) I suggest you search for questions explaining what to use dynamically allocated objects for. I'm sure it's been asked many times before. (If you find answers that do _not_ first and foremost say that stack object are to be preferred whenever possible, then those answers are wrong.) – sbi Feb 20 '11 at 00:48
  • As mentioned in other comments, it's just bad style. At least you should be returning the string as pass-by-reference, e.g arguments(vector *retval), and maybe use retval->swap(my_vec) as a quick way to return it. There's nothing wrong with dynamic allocation - you should just avoid the pattern of returning allocated objects. I'd recommend a read of 'Effective C++' for starters. – John Ripley Feb 20 '11 at 00:50
  • 1
    The for loop fails because because _arguments is a char** so sizeof(_arguments) just returns 8 (or 4 on outdated 32 bit systems) instead of the size of the array. Since it's null-terminated I just used a do-while until the result was null. – jstm88 Feb 20 '11 at 08:24
  • Also, sbi, to your point 2, you did say doing that elsewhere likely messed up the heap. Obviously that wasn't the problem, but you did say that in your response above. Also, as for the dynamic vector, passing an entire array by value is absurd. Adding in stack objects introduces additional complexity because what I'm doing requires having pointers to the arrays to pass them around efficiently. If I did anything on the stack it would just have to be converted to a heap object immediately afterward. Creating all heap objects is the only sensible way to go in this case. – jstm88 Feb 20 '11 at 08:33
  • Also, John, I'm not sure what the purpose of swap would be in this case. I have existing pointers to the original array that need to remain valid after the new one is returned; swapping the contents would not work. Furthermore, passing return values to pointer parameters like that should only be done in rare cases; there are very few situations where that kind of "style" should be used. It just makes for messy code, and it's another thing that's caused innumerable issues when I did try to do something similar before. – jstm88 Feb 20 '11 at 08:41
  • CORRECTION: In the comment about the for() loop above, Markdown replaced my `char**` with `char*` and it won't let me edit it. For clarification, the `_arguments` variable is a `char**`. – jstm88 Feb 20 '11 at 08:43
  • 3
    1) Well, we weren't shown `_arguments`, so we couldn't reason about it. 2) When managing dynamically allocated resources manually, you are likely to make errors, which mess up the stack. Messing up the stack is the only reason for `new` crashing I have seen in 20 years of C++. 3) In C++, you [pass arguments per _reference_, and `const`, if possible](http://stackoverflow.com/questions/2139224/2139254#2139254). _All in all you are looking at C++ from a very ignorant POV without considering your ignorance as limiting your ability to judge._ __Learn, rather than judge.__ – sbi Feb 20 '11 at 10:07
  • sbi, it's obvious now that you made a very arrogant assumption. Had you simply asked me if I had made a mistake with memory management instead of automatically assuming that I did, this could have been avoided. I took you somewhat seriously because of your reputation (if there was indeed an issue with dynamic vectors I wanted to know about it) but clearly I shouldn't have taken you seriously at all. When you answer a question, never assume things like that; you'll end up sending people on a wild goose chase for an answer to a problem that wasn't there to begin with. – jstm88 Feb 20 '11 at 19:47
  • 1
    If there is anything arrogant at all here it is your last assertion: you've basically said, "if you had just asked me if I was making mistakes, I could have told you that I wasn't." Really? I wish I could be that certain about whether I've made a mistake. @sbi is right: if you are trying to manage memory manually in a nontrivial C++ program, it's all but certain that you have a mistake somewhere in your code. The problems may not be readily apparent and may not manifest themselves as observable bugs for a long time, but they're almost certainly there. – James McNellis Feb 20 '11 at 22:29
  • I'm actually referring to a minimal example, so it's my fault it comes across like that. My point is that if he would have suggested that improper memory management is the reason not to use dynamic objects, I wouldn't have had the idea that he was suggesting that dynamic allocations themselves can cause errors. Simply asking me to look at it for errors would have been sufficient. Something along the lines of "This sounds a lot like a memory management issue; you might want to check for errors in your memory management code because that can cause heap corruption" would have been fine. – jstm88 Feb 20 '11 at 22:39
  • Similarly, something like "It's better style to use stack objects or smart pointers because of X and Y reasons. There's a good book mentioned in Z discussion that you might want to take a look at" sounds a lot more professional and a lot less condescending. – jstm88 Feb 20 '11 at 22:42
3

The int count = sizeof(_arguments); seems pretty dubious.

sizeof gives you size in bytes, not number of elements an array (except in the case where each element is exactly one byte, i.e. char).

Cheers & hth.,

Cheers and hth. - Alf
  • 142,714
  • 15
  • 209
  • 331
  • Exactly; that was exactly what the problem was (as I had already stated in my other answer, but it's good to see somebody actually answering the question!) Kudos to you, sir. :) – jstm88 Feb 20 '11 at 23:29
2

Possibly something else has corrupted the heap before you do this call. Have you tried running under valgrind or equivalent?

Chris Card
  • 3,216
  • 20
  • 15
  • I didn't need to - it turns out it was showing the error on this line simply because GDB was confused about where the error actually was. It was actually a logic error in the for() loop that I was able to fix. – jstm88 Feb 19 '11 at 23:04