Segmentation error when altering a variable in an array

Question

I'm trying to develop a cipher program that alters someones text input according to a string of 26 char's they can customise. The route I've decided to go is to subtract the individual char values in their input string from another string of the alphabet. The numerical result would then be stored in a separate int array, which will be applied to all future inputs they want to cipher. In order to make it easier for myself, I am capitalising the 26 char user input, but when my code gets to 'j' in the sample user input I've used (the only letter I didn't capitalise for testing purposes) I get a segmentation error.

The code can be found below. My apologies if I have not been sufficiently descriptive. I'm still pretty new to coding, so will answer any questions as best I can.

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


int main(void)
{

    string x = "QWERTYUIOPASDFGHJKLZXCVBNM"; // sample user input for cipher
    
    string y = "ABCDEFGHIjKLMNOPQRSTVUWXYZ";
    
    int code[25];
    
    for(int i = 0; i < 26; ++i)
    {
        
        if ((int) y[i] > 90)   // For capitalisation
        {
            y[i] -= 32;
        }
        
        code[i] = (int) y[i] - (int) x[i];  // creates cipher array

    }
}

You have int code[25] so the largest valid index for code is 24 (the range is 0 to 24). Your for loop takes i to a value of 25, then attempts access to code[i] which is invalid. — lurker
– lurker, Commented Jun 28, 2020 at 0:36
@lurker I just adjusted that, thanks for the heads up. Unfortunately that is not the cause of the segmentation error, as it is still occurring. — Disierd
– Disierd, Commented Jun 28, 2020 at 0:44
@Dal: string is just a typedef for char*, from the infamous Cambridge CS50. When you assign to y[i] you are modifying a static string and that is a must-not in C. Try declaring the variable as char y[] = "ABC...";. — rodrigo
– rodrigo, Commented Jun 28, 2020 at 0:44
The problem would be completely obvious if you replace string with char * -- it would be absolutely clear that x and y point to constants and therefore you shouldn't modify the thing they point to. If you were taught to use string this way, you should seriously try to find a different teacher because they are teaching you things that make coding significantly harder. — David Schwartz
– David Schwartz, Commented Jun 28, 2020 at 1:33

BEN1JEN · Accepted Answer · 2020-06-28 01:30:47Z

There are two issues with the code:

You are looping 26 times, starting at 0, and ending at 25, but your code array is only 25 items long int code[25], meaning the highest index is 24 (because 0 is the first index). To fix this simply change the length of the code array to 26.
The variable y is actually a pointer to read-only data stored in your program's data (bss) section, this means that if you try to modify it (which isn't happening in this case, but if you had any lowercase letters in y it would), you are trying to write to read-only memory which would cause another segfault. To fix this you either need to copy the data into a read-write array, or (what I've done) simply store the uppercase version in a temporary variable.

Additionally, I would recommend changing the capitalization check to if (y[i] > 'Z') for clarity, as that would have the same effect but would be easier to refer back to later. I've also renamed the variables x and y to better indicate their purpose, and capitalized x instead of y because it's the variable containing user input. Finally, I replaced 'string' with 'char const *' to indicate to the compiler that we shouldn't be allowed to change these values later (string is really just char *).

#include <stdio.h>

int main(void) {
  char const * cipher_alphabet = "QWERTYUIOPASDFGHJKLZXCVBNM"; // sample user input for cipher    
  char const * alphabet = "ABCDEFGHIjKLMNOPQRSTVUWXYZ";
  int code[26];

  for(int i = 0; i < 26; ++i) {
    char capitalized_letter = cipher_alphabet[i];
    if (capitalized_letter > 'Z') { // capitalize
      capitalized_letter -= 32;
    }
    code[i] = (int)alphabet[i] - (int)capitalized_letter;  // create cipher array
  }
}

thealienthing · Accepted Answer · 2020-06-28 01:24:18Z

I would either use c-strings or plain old char arrays to store your strings. I did it below and change them to char array's and that fixed everything.

#include <stdio.h>


int main(void)
{

    char x[] = "QWERTYUIOPASDFGHJKLZXCVBNM"; // sample user input for cipher

    char y[] = "ABCDEFGHIjKLMNOPQRSTVUWXYZ";

    int code[26];

    for(int i = 0; i < 26; ++i)
    {

        if ((int) y[i] > 90)   // For capitalisation
        {
            y[i] -= 32;
        }

        code[i] = (int) y[i] - (int) x[i];  // creates cipher array

    }
    for(int i = 0; i < 26; i++) {
        //added this to see what everything looks like after the work is done
        printf("%d",code[i]);
    }
}

Also something that is helpful for avoiding over indexing an array is to declare a variable and use that for iteration like so:

#define ARRAY_SIZE 26

char my_array[ARRAY_SIZE];
for(int i = 0; i < ARRAY_SIZE; i++)
    //do stuff with array

Collectives™ on Stack Overflow

Segmentation error when altering a variable in an array

2 Answers 2

Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related