1

I'm trying to write a function that takes in two strings, concatenates them, and then writes them to a 2D array. I have been messing around with different things and looking at other posts to no avail. The seg fault seems to occur in the last strcpy when I'm trying to write the the concatenated string to the char array. If that line is commented out the program seems to work just fine.

void saveToArray(char *command1, char *command2)
{
    char *saveCommands[30][100];
    int *commands = malloc(sizeof(int));
    char concatCommand[30];

    strcpy(concatCommand, command1);
    strcat(concatCommand, " ");
    strcat(concatCommand, command2);
    strcpy(*&saveCommands[0][*commands], concatCommand);
    *commands += 1;
}

I apologize in advance for any formatting issues as this is my first post and thanks for any answers!

sampson7185
  • 23
  • 4
  • 1
    `*&` - if that doesn't raise your eyebrow i don't know what will. If this is intended to save anything it won't, none of the vars are static. further, there is only one dynamic allocation in this entire thing, and it isn't even needed. You need to allocate space for your target strings. [Worth reviewing](http://pastebin.com/Sd3PyAzC). – WhozCraig Feb 13 '15 at 01:57
  • 1
    `strcpy(*&saveCommands[0][*commands]` - what the sam hill – M.M Feb 13 '15 at 02:36
  • 1
    @MattMcNabb I thought that was pretty cool myself. :) The `*commands` is a particularly nice touch since it never has a value set, it's simple allocated memory. The `*&` has got to be my favorite though. :) – David Hoelzer Feb 13 '15 at 02:38

2 Answers2

1

There are a number of scary things going on in this function. For starters...

int *commands = malloc(sizeof(int));

You don't free this or return it, so it is 100% a memory leak. I see no reason why you should want to allocate this integer on the heap rather than the stack.

char concatCommand[30];

strcpy(concatCommand, command1);
strcat(concatCommand, " ");
strcat(concatCommand, command2);

This could potentially overflow your buffer.

strcpy(*&saveCommands[0][*commands], concatCommand);

*& is a no-op (it doesn't do anything). You don't copy anything into commands, so *commands could be anything. Finally, the pointers in saveCommands aren't initialized so strcpy(saveCommands[x][y], "some string") will basically always segfault in practice. I think you might want this instead:

char saveCommands[30][100]
...
strcpy(&saveCommands[0][*commands], concatCommand);
Peter Enns
  • 600
  • 5
  • 8
0

I took the liberty to re-write your code a bit... too many characters make my eyes hurt... at any rate, this code works. It was mainly the strcpy dereference that was causing the issue.

#include <stdlib.h>
#include <stdio.h>
#include <string.h>

void cmds(char *c1, char *c2) {
    char  sc[30][100];
    int   c = 0;
    char  cc[30];

    strcpy(cc,  c1);
    strcat(cc, " ");
    strcat(cc,  c2);
    strcpy(&sc[c][0], cc); // problem was here...
    printf("%s\n", &sc[c][0]);
}

int main() {
    char *c1 = "c001";
    char *c2 = "c002";

    cmds(c1, c2);
    return 0;
}
Travis Rodman
  • 607
  • 1
  • 6
  • 14
  • I have a feeling that `strcpy(&sc[c][0]` (aka. `strcpy(sc[c]`) is OP's intent , so that he can introduce a loop to store subsequent strings without overwriting the first one – M.M Feb 13 '15 at 02:39
  • you are probably right... I will modify that for reference. – Travis Rodman Feb 13 '15 at 02:41
  • yeah that's what I was going for, I'm trying to store each command in a subsequent element of the array, thanks a bunch for taking the time to answer, it helped out a lot! – sampson7185 Feb 13 '15 at 02:47