What's wrong with my c code? (it's probably the malloc call)

Question

When I run this code, I get a segmentation fault. I'm sure I'm doing the pointers wrong, but I'm not sure why or how to fix it. I also included the question I'm trying to answer in picture form.

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

char * repeat_characters(char *s, int n) {
    printf("%s", *s);

    char temp;
    int length = 0;
    int i = 0;
    int j = 0;
    int k = 0;
    char * newString;

    while(s[length]) {
        length++;
        printf("%d", length);
    } // finds length of string

    newString = (char*) malloc(length*n*sizeof(char));

    while(i++ < i*n) {
        temp = s[i];
        while (j++ < n) {
            newString[k] = temp;
            printf("%c", temp);
            k++;
        }
    }
    printf("%s", newString); 
    return newString;
}

int main () {
    char * string[100];
    int numReps = 0;

    printf("Enter a string: ");
    scanf("%s", string);

    printf("\nEnter number of repetitions: ");
    scanf("%d", &numReps);

    repeat_characters(*string, numReps);
    return 0; 
}

Answer 1

One of your jobs as a questioner is to shorten your program to the minimum that demonstrates a problem. For instance, the following would give a segmentation fault:

#include <stdio.h>

char * repeat_characters(char *s, int n) {
    printf("%s", *s);
    return NULL;
}

int main () {
    char * string[100];
    scanf("%s", string);

    repeat_characters(*string, 0);
    return 0; 
}

Lots of kinds of problems could be pointed out by compiler warnings. So turn those warnings on. If you're using gcc, then try something like gcc -Wall -Wextra -Werror main.c (or look in your compiler's documentation.)

(The -Werror will turn the warnings into errors, and keep you from accidentally ignoring a warning, which I'd say is a good habit for both learners and experts alike.)

   printf("%s", *s);

format '%s' expects argument of type 'char*', but argument 2 has type 'int'

Here you have a parameter char *s. Depending on context could either be a pointer to a single character, or a pointer to the first character of a multiple-character sequence.

C strings operate on the convention that it's a multiple character sequence, and that the range of characters is eventually terminated by a '\0' (or 0-valued character). So when you call printf you must be sure that the character pointer you pass in is to such a validly formed sequence.

You are passing in *s, which dereferences the pointer-to-a-character to make it into a single character. If you want to print a single character, then you'd need to use %c, e.g. printf("%c", *s);. But you want to print a C string, so you should drop the dereferencing * and just say printf("%s\n", s). (You will likely want \n to output a newline, because otherwise your prints will all run together.)

(Note: The reason C without warnings didn't complain when you passed a character where a character pointer was expected is due to the "weirdness" of functions like printf and scanf. They don't have a fixed number or type of arguments that they take, so there is less checking...warnings help pick up the slack.)

   scanf("%s", string);

format '%s' expects argument of type 'char*', but argument 2 has type 'char**'

Here you have the problem that you've declared s as char * string[100];, which is an array of character pointers, not an array of characters. The duality of a C array being able to behave like a pointer to its first element takes some getting used to, but if you say char string [100]; then string[0] is a char and string can "decay" to a char* synonymous with &string[0]:

Is an array name a pointer?

   while(i++ < i*n)

operation on 'i' may be undefined

The compiler is making a technical complaint here, about modifying the variable i in an expression that also uses i. It's warning about whether the i in i*n would see the value before or after the increment. Avoid this kind of expression...and when-and-if the time comes where you care about what you can and can't do like this, read about sequence points.

But put that aside. What did you intend here? Take the nuance of ++ out of it...what about just while (i < i * n)? When would that be false? Why would it be relevant to solving the problem?

Look through your code and perhaps try commenting it more. What are "the invariants", or the things that you can claim about a variable that would be true on each line? If you were asked to defend why the code worked--with no compiler to run it to see--what would be giving you the certainty that it did the right thing?

Then step through it with a debugger and examples, and check your intuition. If you step over a line and don't get what you expect, then that's when it may be time to start thinking of a question to ask which draws the focus to that line.

There's other issues raised in the comments (for instance, that your malloc() doesn't include space for the terminator, and even once you add in 1 for that space you still have to write a '\0' into it...). Generally speaking your program should perform as many free()s as it does malloc()s. Etc. But hopefully this points you in the right direction for the assignment.