Hello, OnlineGDB Q&A section lets you put your programming query to fellow community users. Asking a solution for whole assignment is strictly not allowed. You may ask for help where you are stuck. Try to add as much information as possible so that fellow users can know about your problem statement easily.

Pointer to Pointer in Function segmentation error

+3 votes
asked Jan 14 by Tom Stevens - TWCC - (230 points)

Hi, Could anyone please explain to me the correct syntax for passing an array of character pointers to a function,

which is then called by an function?

I've emboldened the code which causes the segmentation fault.

The intent is to call one function from another using a pointer to character arrays.

Thank you for your help.

#include <stdio.h>
#include <stdlib.h>
#include <ctype.h>
#define ARRAY_SIZE 20
#define POTION_NUMBER 3

void show_inventory();
char potion_choice();
void code_saver3();

int main()
{
    char potion_select;
    char *potion[POTION_NUMBER];
    char *inventory[ARRAY_SIZE];
    int i;
    
    for (i = 0; i < POTION_NUMBER; i++)
        potion[i] = (char *)malloc(40);
        
    for (i = 0; i < ARRAY_SIZE; i++)
        inventory[i] =(char *)malloc(50);
    
    for(i = 0; i < ARRAY_SIZE; i++)
        inventory[i] = "";
        
    inventory[0] = "Sword";
    inventory[1] = "Leather Armour";
    inventory[2] = "Magic Potion";
    inventory[3] = "Shield";
    inventory[4] = "Back-pack";
        
    potion_select = potion_choice(&potion, potion_select);
    
    
     switch (potion_select)
    {
        case 'K': printf("%s selected\n", potion[0] );
                  inventory[2] = potion [0];        // set inventory to skill
                 break;
        case 'S':  printf("%s selected\n", potion[1] );
                  inventory[2] = potion[1];         // set inventory to stamina
                 break;
        case  'L': printf("%s selected\n", potion[2] );
                   inventory[2] = potion[2];       // set inventory to luck
                 break;
    }
    show_inventory(&inventory);
    
    code_saver3(&inventory);  // Incorrect pointer to pointer to  function here
                              // causes segmentation fault, please help!
                              
                              // the intention is to go code_saver3 - show_inventory - output inventory
                              // variables.
    

    return 0;
}
char potion_choice(pnta, pot_sel)
char *(*pnta), pot_sel;
{
    
    *pnta = "Potion of Skill";
    *pnta++;
    *pnta = "Potion of Stamina";
    *pnta++;
    *pnta = "Potion of Luck";
    
    printf("Please choose a magic potion: \n");
    printf("1: S(K)ill\n");
    printf("2: (S)tamina\n");
    printf("3: (L)uck\n");
    do
    {
        pot_sel = getchar();
        pot_sel = toupper(pot_sel);
    } while (pot_sel != 'K' && pot_sel != 'S' && pot_sel != 'L');
    return pot_sel;
}

void show_inventory(pnta)
char *(*pnta); 
{
    while (*pnta != "")
    {
        printf("%s\n", *pnta);
        *pnta++;
    }
    
}


void code_saver3(pntaa)

char *(*pntaa);
{
    show_inventory(*pntaa);
    
}

2 Answers

+1 vote
answered Jan 15 by Peter Minarik (19,180 points)
edited Jan 16 by Peter Minarik

Does Your Code Not Compile?

The problem is that you're using some age old function notation that is only acceptable in C compilation and you compile your code as C++ (check the top right corner).

I'd suggest getting rid of this peculiar style and use the C++ compatible parameter definition.

So, instead of 

void code_saver3(pntaa)
char *(*pntaa);
{
    show_inventory(*pntaa);
}

use

void code_saver3(char ** pntaa)
{
    show_inventory(*pntaa);
}

Apply the same logic for all of your functions.

Note

After this, your code compiles, but it has further problems.

Errors to Fix

Inventory

for (i = 0; i < ARRAY_SIZE; i++)
    inventory[i] =(char *)malloc(50);

for(i = 0; i < ARRAY_SIZE; i++)
    inventory[i] = "";

inventory[0] = "Sword";
inventory[1] = "Leather Armour";
inventory[2] = "Magic Potion";
inventory[3] = "Shield";
inventory[4] = "Back-pack";

So, in the first loop, you allocate memory for ARRAY_SIZE (20) C-strings in inventory.

In the second loop, you set inventory elements to point to the string literal "" (empty string). By this, you've lost pointers to the previous allocation (malloc). This is a memory leak.

Even worse, if anyone tries to write any inventory element (, which are right now string-literals, that mustn't be changed -- read: they are read-only) then it leads to a Segmentation Fault. (Memory corruption.)

Later, you set various inventory elements (0..4) to various string literals. (Again, we've already got memory leaks and this could be a source of problems if anyone tries to change the values stored in inventory.)

At this point, I'd advise that you make a decision:

  1. inventory is an array of buffers that you can write (sprintfs(buffer, ____)) OR
  2. inventory is an array of pointers to constant strings (const char ** inventory) where you set every pointer to point to an existing string that you mustn't change later.

I'd go with the second one as I think it's fine to set the inventory elements to point to various names. If you need to change a name, you can just point it to a new string. But it's up to you. Just know what you're doing.

potion_choice()

char potion_select = potion_choice(&potion, potion_select);
// ...
char potion_choice(char ** pnta, char pot_sel)
{
    
    *pnta = "Potion of Skill";
    *pnta++;
    *pnta = "Potion of Stamina";
    *pnta++;
    *pnta = "Potion of Luck";
    
    printf("Please choose a magic potion: \n");
    printf("1: S(K)ill\n");
    printf("2: (S)tamina\n");
    printf("3: (L)uck\n");
    do
    {
        pot_sel = getchar();
        pot_sel = toupper(pot_sel);
    } while (pot_sel != 'K' && pot_sel != 'S' && pot_sel != 'L');
    return pot_sel;
}

As you can see, in the first line you set postion_select to the return value of potion_choice, while also being an input (but not output!) parameter of the same method.

I'd start the clean-up job by removing the second parameter (potion_select) from the potion_choice method. Instead, I'd declare potion_select as a local variable.

Furthermore, what this method is supposed to do (based on the name) is to ask a user input for selecting a potion. This method shouldn't manipulate the potions array (pnta). I would set up these values where the rest of the initialization happens.

Also, you have an array that stores the various potions, yet you display static values, not what the actual potions array contains.

One serious problem is with the calling of the method. You did 

char potion_select = potion_choice(&potion, potion_select);

where you get the address of potion, which is a char ***, instead of char **. Then you write this array, which leads to another memory corruption. Remove the "address of" (&) operator.

show_inventory()

Similarly to the above, show_inventory() is called with the wrong argument type (char ***, instead of char **).

show_inventory(&inventory);

You should remove the "address of" (&) operator in this case as well.

The method itself also has problems.

void show_inventory(const char ** pnta)
{
    while (*pnta != "")
    {
        printf("%s\n", *pnta);
        *pnta++;
    }
}

The problem here is that you keep going as long as *pnta (i.e. inventory[i]) is not empty. But what if your whole inventory is full? Then you'll keep reading memory after the inventory, which you have no business touching. This way, you'll read memory garbage.

I'd add an extra check if you haven't read and printed more than ARRAY_SIZE elements.

code_saver3()

When you call this method, you have the same problem with the above two cases: wrong pointer type. The same fix can be used.

The Amended Code

I'll post it in another answer as I've reached the maximum character length. :)

+1 vote
answered Jan 15 by Peter Minarik (19,180 points)

The Amended Code

Below is the code with fixes I've applied for you.

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

#define ARRAY_SIZE 20
#define POTION_NUMBER 3

static void show_inventory(const char ** pnta);
static char potion_choice(const char ** pnta);
static void code_saver3(const char ** pntaa);

int main()
{
    //
    // Initialization
    // 
    const char *potion[POTION_NUMBER] =
    {
        "Potion of Skill",
        "Potion of Stamina",
        "Potion of Luck"
    };
    
    const char *inventory[ARRAY_SIZE];

    int i = 0;
    inventory[i++] = "Sword";
    inventory[i++] = "Leather Armour";
    inventory[i++] = "Magic Potion";
    inventory[i++] = "Shield";
    inventory[i++] = "Back-pack";

    for(; i < ARRAY_SIZE; i++)
        inventory[i] = "";
        
    //
    // Usering input for potion choice
    //
    switch (potion_choice(potion))
    {
        case 'K': inventory[2] = potion[0]; break; // set inventory to skill
        case 'S': inventory[2] = potion[1]; break; // set inventory to stamina
        case 'L': inventory[2] = potion[2]; break; // set inventory to luck
    }
    printf("\n%s selected\n\n", inventory[2]);

    //
    // Display the inventory
    //
    show_inventory(inventory);

    code_saver3(inventory);
    return 0;
}

char potion_choice(const char ** pnta)
{
    printf("Please choose a magic potion:\n");
    printf("1: S(K)ill\n");
    printf("2: (S)tamina\n");
    printf("3: (L)uck\n");

    char pot_sel;
    do
    {
        pot_sel = toupper(getchar());
    } while (pot_sel != 'K' && pot_sel != 'S' && pot_sel != 'L');

    return pot_sel;
}

void show_inventory(const char ** pnta)
{
    printf("Inventory:\n");
    printf("----------\n");
    for (int i = 0; i < ARRAY_SIZE; i++)
    {
        if (pnta[i] == "")
            break;
        
        printf("%s\n", pnta[i]);
    }
    printf("\n");
}

void code_saver3(const char ** pntaa)
{
    show_inventory(pntaa);
}
commented Jan 15 by Tom Stevens - TWCC - (230 points)
Awesome! Thanks for your help.
commented Jan 16 by Peter Minarik (19,180 points)
My pleasure. :)
commented Jan 17 by Tom Stevens - TWCC - (230 points)
Hi again. You might have gathered this is part of a text based game I'm developing.
Are the strings in the inventory strictly read only?
It's just that in the game the potion is 'drunk' and is thus removed from the array.
My intention is to set an 'empty' element in the array after the player has chosen this option.
commented Jan 17 by Peter Minarik (19,180 points)
As explained above, you can either have the elements in the "inventory" be pointers to read-only string (const char **) and only set the individual "inventory" items to point to various strings.

The other option is to have a character buffer array 9 (char *inventory[]) and you are free to change that. But in that case, you shouldn't modify the where the pointers point (and you have to allocate memory and free in the end).

The important thing is to be consistent.

You can remove a potion by setting the value in the inventory to "" (strcpy(inventory[i], "")) if you use a buffer.

If you use pointers, you should point to an empty string (invenotry[i] = "").
Welcome to OnlineGDB Q&A, where you can ask questions related to programming and OnlineGDB IDE and and receive answers from other members of the community.
...