Structs, strtok, segmentation fault

Question

I'm trying to make a program with structs and files. The following is just a part of my code(it;s not the entire program). What i'm trying to do is: ask the user to write his command. eg. delete John eg. enter John James 5000 ipad purchase.

The problem is that I want to split the command in order to save its 'args' for a struct element. That's why i used strtok. BUT I'm facing another problem in who to 'put' these on the struct. Also it's seems quite odd to me how to 'pass' the 'args' to the struct in a safe way,as I save all enters (from user) on a binary file which maybe be reopened and rewrited so I cant use :

strcpy(catalog[0]->short_name, args[1]); 

Because it's time the short name will be saved in the first element of the struct. But if the file is written what happens then? The first element exists so if i write ..[0] i will write up on it? What should I do? Thanx in advance for any help! :D

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define MAX 100

char command[1500]; 

struct catalogue                
{
        char short_name[50];
        char surname[50];
        signed int amount;
        char description[1000];
}*catalog[MAX]; 

int main ( int argc, char *argv[] )
{
    int i,n;
    char choice[3];

    printf(">sort1: Print savings sorted by surname\n");
    printf(">sort2: Print savings sorted by amount\n");
    printf(">search+name:Print savings of each name searched\n");
    printf(">delete+full_name+amount: Erase saving\n");
    printf(">enter+full_name+amount+description: Enter saving \n");
    printf(">quit:  Update + EXIT program.\n");

    printf("Choose your selection:\n>");
    gets(command);                     //it save the whole command

    /*in choice it;s saved only the first 2 letters(needed for menu choice again)*/
    strncpy(choice,command,2);      
    choice[2]='\0';                   

char** args = (char**)malloc(strlen(command)*sizeof(char*));
memset(args, 0, sizeof(char*)*strlen(command));

char* temp = strtok(command, " \t");

for (n = 0; temp != NULL; ++n)
{   
    args[n] = strdup(temp);
    temp = strtok(NULL, " \t");
    printf(" %s ",args[n]);
}

strcpy(catalog[0]->short_name, args[1]);         //segmentation fault
strcpy(catalog[0]->surname,args[2]);
catalog[0]->amount=atoi(args[3]);               //atoi doesn't work
strcpy(catalog[0]->description,args[4]);


}

As a result, after running the program i get a Segmentation Fault... for the line:

strcpy(catalog[0]->short_name, args[1]); 

Any help? Any ideas?

Answer 1

The for loop assigns one argument at a time (args[n] = ...), but then accesses several arguments on each pass: *args[1], args[2], etc., which are uninitialized on the first pass.

The warning is due to another bug. You can't just assign a pointer to an array like that. Use strcpy() instead.

Answer 2

your array for catalouge is an array of pointers, not an array of objects, but those pointers aren't initialized to anything, hence the seg fault

try:

struct catalogue                 
{ 
        char short_name[50]; 
        char surname[50]; 
        signed int amount; 
        char description[1000]; 
}catalog[MAX]; 



strcpy(catalog[0].short_name, args[1]);         //segmentation fault  
strcpy(catalog[0].surname,args[2]);  
catalog[0].amount=atoi(args[3]);               //atoi doesn't work  
strcpy(catalog[0].description,args[4]); 

Answer 3

You have 2 errors:

1. Your catalog[MAX] array holds MAX pointers to your struct catalogue, but none of them are initialized. The way to fix this is to either not declare them to be pointers, or to malloc them as needed, like in catalog[0] = (struct catalogue *)malloc(sizeof(struct catalogue));

2. Your args variable is bad. First, I don't think you intend to create an array of strings whose length is the length of your command string. That means if you type "sort1" you will create args[5]. That's nonsensical, because the length of your command has nothing to do with how many arguments it should have.

   But assuming you really want to do that, you are creating space for the array, but not for the strings within the array. You will get a segfault eventually anyway (though the one you are getting is due to #1 above) because of that. You need to allocate space for each element in args as you use it.

The code could look like this:

for (n = 0; temp != NULL; ++n)
{
   args[n] = (char *)malloc((strlen(temp) + 1) * sizeof(char));
   strcpy(args[n], temp);
   // and so on
}

Answer 4

Lots of problems in this code.

First of all, you're confusing the number of arguments on the input line for the number of entries in the catalog. In one context you're using n to count the number of args, but in another you're using it to index the catalog array.

You're creating memory management headaches where you don't need to. The args variable is completely unnecessary, and you're allocating the memory for it incorrectly anyway. You're basically saying, "allocate a pointer to char for every character in command, which is probably not what you want.

Lose args completely; you don't need it.

I realize this is not your entire program, but it's not clear why you're creating catalog as an array of pointer to struct catalog as opposed to just a regular array.

I'm not sure what you think you're doing on the line

*catalog[n]->short_name=*args[1]; 

The type of the expression catalog[n]->short_name is char[50]. In this context the array type is implicitly converted ("decays") to a pointer type, char *. Thus the type of the whole expression *catalog[n]->short_name is * (char *), or just plain char, which is an integral type. You're essentially trying to assign the value of the first character of args[1] to the first character of catalog[n]->short_name.

None of this matters anyway, because catalog[n] hasn't been initialized to point anywhere meaningful; the segfault is coming from the attempt to access the short_name member, which implicitly dereferences catalog[n], which is pointing somewhere random.

Next, you cannot use the assignment operator = to assign string data; you must use strcpy() or strncpy() to do that.

Finally, NEVER NEVER NEVER NEVER NEVER use gets(). It will introduce a point of failure in your code. It has been officially deprecated in C99 and should no longer be used. Use fgets() instead:

if (fgets(command, sizeof command, stdin) != NULL)
{
  char *newline = strchr(command, '\n');
  if (newline != NULL)
    *newline = 0;
}

Here's the way you need to parse out the command string and assign the fields to members of the struct:

curToken = strtok(command, '\t');
if (curToken)
  strncpy(catalog[n]->short_name, curToken, sizeof catalog[n]->short_name);

curToken = strtok(NULL, '\t');
if (curToken)
  strncpy(catalog[n]->surname, curToken, sizeof catalog[n]->surname);

curToken = strtok(NULL, '\t');
if (curToken)
{
  char *chk;
  catalog[n]->amount = (int) strtol(curToken, &chk, 10);
  if (!isspace(*chk) && *chk != 0)
    fprintf(stderr, 
      "Warning: expected integer value for amount, received %s instead\n",
      curToken);
}

curToken = strtok(NULL, '\t');
if (curToken)
  strncpy(catalog[n]->description, curToken, sizeof catalog[n]->description);

This code assumes that catalog is still being declared as an array of pointer and that each element has been initialized to point somewhere meaningful. Otherwise change the declaration from struct catalog {...} *catalog[MAX]; to struct catalog {...} catalog[MAX] and change -> to ..