0

I am getting a segmentation fault from the below program.

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

void removeProcess(int*, int);
void removeProcessN(char**, int, int);

void main() {
    int numPro = 0, quanTime = 0, contTime = 0, i, elemNum, time = 0;

//Supply variables with user input
    printf("Enter number of processes: ");
    scanf("%d", &numPro);
    printf("Enter context switch time: ");
    scanf("%d", &contTime);
    printf("Enter quantum of time: ");
    scanf("%d", &quanTime);

//Create array of number of process time
    int proTime[numPro];
    //Create string array for better output
    char *proNames[numPro];

//Retrieves process time from user
    for (i = 0; i < numPro; i++){
        printf("Enter execution time for process %d: ", i);
        scanf("%d", proTime + i);
        sprintf(proNames[i], "p%d", i);
    }

    elemNum = 0;


//While a process remains active
    while (numPro != 0) {
//Retrieves the element being worked with
        elemNum = elemNum % numPro;

//Describe process working with
        printf("Executing process %s\nStart time = %d\n", proNames[elemNum], time);

        proTime[elemNum] -= quanTime;

//If process time complete, remove process
        if (proTime[elemNum] <= 0){
            removeProcess(proTime, elemNum);
            removeProcessN(proNames, elemNum, numPro);
            --numPro;
        }

//Add amount of time with context time
        time = time + quanTime + contTime;
        elemNum++;
    }
}

/**
*@param *array pointer to an array of integers
*@param elem int of the element to remove
* Removes an element 'elem' from the supplied integer array.
*/
void removeProcessN(char **array, int numElem, int elem) {

    char *temparray[numElem - 1];

//Copy array to temparray except for elem to remove
    int i;
    for (i = 0; i < elem; i++) {
        if (i == numElem) {
            continue;
        } else {
            temparray[i] = array[i];
        }
    }

//End by setting the pointer of array to the temparray
    array = temparray;
}

/**
*@param *array pointer to an array of integers
*@param elem int of the element to remove
* Removes an element 'elem' from the supplied integer array.
*/
void removeProcess(int *array, int elem) {
//Number of elements in the array
    int numElem = sizeof(array) / sizeof(int);

    int temparray[numElem - 1];

//Copy array to temparray except for elem to remove
    int i;
    for (i = 0; i < numElem; i++) {
        if (i == elem) {
            continue;
        } else {
            temparray[i] = array[i];
        }
    }

//End by setting the pointer of array to the temparray
    array = temparray;
}

I know the segmentation fault is coming from sprintf. I am trying to simulate how an operating system would complete a process using round robin. I have tried using sprintf because that's what tutorials were saying online to use when trying to manipulate strings. The removeProcessN is just removing an index from the array proNames. I am mostly just concerned with the sprintf.

I have tried malloc when I do the sprintf but it would not even compile at that point. If someone could offer an explanation I'd be appreciative.

1
  • You need to allocate both the array with the appropriate number of string pointers and its values so they point to a real buffer. Commented Feb 23, 2018 at 0:38

1 Answer 1

3

The problem here is that proNames is an array of pointers, but they are uninitialized, so passing it to sprintf to write something, will crash. You would have either use a double array or allocate memory with malloc. But as you are only printing integers and the string representatuion of integers has a maximal length, allocating memory with malloc will be more harder, because you have to check that malloc doesn't return NULL, you have to free the memory later, etc.

So I'd do:

char proNames[numPro][30]; // 28 characters for an int (usually 4 bytes long)
                           // should be more than enough

//Retrieves process time from user
    for (i = 0; i < numPro; i++){
        printf("Enter execution time for process %d: ", i);
        scanf("%d", proTime + i);
        sprintf(proNames[i], "p%d", i);
    }

Your removeProcessN would need to change as well:

void removeProcessN(int numElem, int elem, int dim, char (*array)[dim]) {

    for(int i = elem; i < numElem - 1; ++i)
        strcpy(array[i], array[i+1]);

    array[numElem - 1][0] = 0; // setting last element to empty string
}

Note that I moved the array argument at the last position, otherwise numElem is not known and the compiler would return an error.

And now you can call it like this:

removeProcessN(elemNum, numPro, 30, proNames);

The 30 comes from the char proNames[numProp][30]; declaration.

I'd like to comment on the last line of your function removeProcessN:

//End by setting the pointer of array to the temparray
array = temparray;

That is not correct, first because temparray is local variable and ceases to exist when the function returns. And array is local variable in the function, so changing it doesn't affect anybody.

The alternative with memory allocation would look like this:

char *proNames[numPro];

//Retrieves process time from user
    for (i = 0; i < numPro; i++){
        printf("Enter execution time for process %d: ", i);
        scanf("%d", proTime + i);
        int len = snprintf(NULL, 0, "p%d", i);
        proNames[i] = malloc(len + 1);
        if(proNames[i] == NULL)
        {
            // error handling, free the previously allocated
            // memory, and return/exit
        }

        sprintf(proNames[i], "p%d", i);
    }

and removeProcessN:

void removeProcessN(char **array, int numElem, int elem) {

    char *to_remove = array[elem];

    for(int i = elem; i < numElem - 1; ++i)
        array[i] = array[i+1];

    free(to_remove);

    array[numElem - 1] = NULL; // setting last element to NULL
                               // makes freeing easier as
                               // free(NULL) is allowed
}

And the way you originally called the removeProcessN would be OK.

If you eventually call removeProcessN for all processes, then all the memory should be freed because removeProcessN frees it. If there are some elements that remain in the array, then you have to free them later.


OP posted in the comments

My theory was that temparray would be a pointer to an array so I could just remove an index from the main array. So when I say array = temparray, the pointer for array points to temparray. I know it worked for removeProcess. Is it different for strings?

The array = temparray also has no effect in removeProcess, array is still a local variable and changing where it points to has no effect at all, because you are changing a local variable only.

Besides the code is wrong:

int numElem = sizeof(array) / sizeof(int);

this only works for pure arrays, it does not work for pointers because sizeof(array) returns you the size that a pointer of int needs to be stored. Like the other function, you need to pass the site the array to the function.

If you say that this function worked, then just only by accident, because it yields undefined behavior. By incorrectly calculating the number of elements, temparray will have the wrong size, so here temparray[i] = array[i]; you may access beyond the bounds which leads to undefined behaviour. Undefined behaviour means that you cannot predict what is going to happen, it could be anything from crashing to formatting your hard drive. Results that result from undefined behaviour are useless.

And again array = temparray; just changes where the local variable array is pointing, the caller of removeProcess doesn't see that.

The correct version would be:

int removeProcess(int *array, int elem, int numElem) {

    if(array == NULL)
        return 0;

    // nothing to do if the elemnt to be removed is
    // the last one
    if(elem == numElem - 1)
        return 1;

    // overwriting the memory, because memory
    // regions overlap, we use memmove
    memmove(array + elem, array + elem + 1, numElem - elem - 1);

    return 0;
}

So, to make it clear:

Let's look at this code:

void sum(int *array, size_t len);
{
    int c[len];

    array = c;
}

void bar(void)
{
    int x[] = { 1, 3, 5 };

    size_t len = sizeof x / sizeof *x;

    sum(x, sizeof x / sizeof *x);
    printf("x[0] = %d, x[1] = %d, x[2] = %d\n", x[0], x[1], x[2]);
}

sum has only a copy of the pointer you've passed in bar, so from bar's point of view, sum changed the copy, so bar will print x[0] = 1, x[1] = 3, x[2] = 5.

But if you want that the caller sees any change, then you to access through the pointer:

void sum(int *array, size_t len)
{
    int c[len];
    for(size_t i = 0; i < len; ++i)
        array[i] += 10;

    array = c;
}

With this version bar would print x[0] = 11, x[1] = 13, x[2] = 15 and and array = c will have no effect on bar.

Sign up to request clarification or add additional context in comments.

3 Comments

My theory was that temparray would be a pointer to an array so I could just remove an index from the main array. So when I say array = temparray, the pointer for array points to temparray. I know it worked for 'removeProcess'. Is it different for strings?
@TylerWeaver I've made an update addressing your comment.
You were right. I created a driver to run my program 10k and record results. It produced undesired/harmful behavior nearly 90% of the time. It was just a coincidence the method was working. Thank you for all of your help.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.