1

I'm tring to make this program work, but after giving it a lot of turns, I decided is time to ask. No matter what I do the result is always a segmentation fault and I pretty much sure that is for the strcmp. Can you help me please?

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

void main ()
{
    char *marcas[7] = {"Alfa Romeo", "Fiat", "Ford", "Lancia", "Renaudl", '\0', '\0'};
    char *marca, *aux;
    int i;
    int cont=5;
    
    marca=" ";
    aux=" ";        
    for(i=0;i<cont;i++)
    printf("%s\n",marcas[i]);
    while (cont<7){
    printf("Ingrese la marca que desee agregar a la lista.\n");
    scanf("%s",marca);
    printf("Gracias. Ud. quiere agregar la marca: %s.\n", marca);
    i=0;
    while(strcmp(marcas[i], marca)<0 && i<cont){
            i++;
            printf("Moviendo puntero.\n");
    }
    printf("Termine de mover.\n");
    if (i < cont){
       while (i<cont){
        strcpy(aux,marcas[i]);
        strcpy(marcas[i],marca);
        strcpy(marca,aux);
        i++;
       }
    } 
    printf("%s tiene que ir en la posicion %d\n", marca, i);
    strcpy(marcas[i],marca);    
    cont++;
    for(i=0;i<cont;i++)
        printf("%s\n", marcas[i]);
    printf("La marca ingresada última a la lista:%s\n", marca);
    }
    for(i=0;i<7;i++)
    printf("%s\n", marcas[i]);
}
Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
Andrea Graf
  • 13
  • 2
  • 2
    The first thing you should always do is enable all the compiler warnings. For example if you're using GCC, you would add the `-Wall` option to the compile line. The compiler will give you a wealth of information about bad practices, or even outright errors in your code. – MadScientist Apr 30 '23 at 21:08
  • 3
    The last two elements of your array, i.e. `'\0'`, are just very misleading ways of writing `NULL` in that context. They are not strings, but are integer constants whose value is the null character, i.e. the value zero, which in a pointer context are the `NULL` pointer. Why are they there? You can't pass those elements to `strcmp` since they're `NULL` pointers. – Tom Karzes Apr 30 '23 at 21:08
  • @MadScientist The only warning `gcc -Wall` gives for this code is the incorrect declaration of `main`, which should return `int` rather than `void`. The `'\0'` constants, while unusual, are simply zero constants which are taken to be `NULL` pointers in that context, with no warning given. – Tom Karzes Apr 30 '23 at 21:13
  • If I enable -O2 I get more warnings (GCC 13) but I was really surprised to not get any warnings about assigning a constant string to a `char*`. GCC must allow this as a special case but it seems very dangerous to me to not warn about it. – MadScientist Apr 30 '23 at 21:20

3 Answers3

2

This statement

scanf("%s",marca);

invokes undefined behavior at least because it is trying to change the string literal pointed to by the pointer marca

marca=" ";

You need to declare a character array where you will read a string.

The same problem exists with the pointer aux used in calls of strcpy

strcpy(aux,marcas[i]);

and with other string literals that are pointed to by elements of the array marcas. SO instead of the array of pointers declare a two-dimensional array initialized by the string literals.

For example

char marcas[7][12] = {"Alfa Romeo", "Fiat", "Ford", "Lancia", "Renaudl", };

Pay attention to that any attempt to change a string literal results in undefined behavior.

Vlad from Moscow
  • 301,070
  • 26
  • 186
  • 335
0

Your program contains many segfaults, due to the fact that c does not have built in variable length strings, as many other programming languages do. This can cause problems with trying to 'overfill' a string.

char *marca, *aux;

In this line, you define a pointer (marca and aux) to a single character each, and so when you call this line:

scanf("%s",marca);

you are reading a potentially huge block of text off the command line, and trying to fit it all into the space of one character. This is not good for the program, and if not dealt with can cause vulnerabilities such as Buffer Overflows. C is an inherently unsafe language, and I recommend learning what you are doing before you attempt to use pointers. A better solution might be something like this:

// Create a list of characters (a string) 128 spaces long called marca, and initialize it.
char marca[128] = {'\0'};
char aux[128] = {'\0'};

In conclusion, the best course of action would be to learn C properly.

WooHooASDF
  • 28
  • 3
  • I know. It make a lot time since I used strings in C and I really forget a lot of things. I must star learning again – Andrea Graf Apr 30 '23 at 21:40
  • marca and aux aren't pointers to single character; they are *uninitialized* pointers, no memory was reserved with malloc nor they are made to point to some other buffer. – ulix May 01 '23 at 08:52
0

One additional note to the other answers is that you can use fgets instead of scanf. The good thing with fgets is that you can limit the number of characters read from the user to whatever you want.

So instead of

scanf("%s", marca);

do

fgets(marca, 128 /*or whatever number you choose*/, stdin);

The above fgets call will only read the first 128 (or whatever number you choose) characters from the user, no matter how many they actually enter. This will prevent buffer overflows.

azhen7
  • 371
  • 1
  • 5