program.exe(c) (malloc) has stopped working

Question

Hi I wrote this code and it worked but in the end, "the program has stopped working"

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

void main() 
{
    char *s;
    s=(char*)malloc(sizeof(char));
    printf("Enter a string:\n");
    gets(s);
    while (*s)
    {
        if (*s>= 65 && *s<=90)
            printf("%c",*s+32);
        else if(*s>=97 && *s<=122)
            printf("%c",*s-32);
        else
            printf("%c",*s);
        *s++;
    }
    free(s);
}

Answer 1

That code does not work, in fact it has undefined behavior.

This:

s = (char *) malloc(sizeof(char));

allocates 1 byte of storage, into which you then scan a string, thus very likely leading to buffer overflow. The buffer can only hold a single string, i.e. string of 0 characters before the terminator character at the end.

You meant:

s = malloc(128);

or something like that.

There's no need to cast, and sizeof (char) is always 1 so that doesn't add anything.

Also, as more of a code review, magic numbers in code is generally considered a bad idea, instead write:

if (*s >= 'A' && *s <= 'Z')

or even better

if (isupper((unsigned int) *s))

to not hard-code a depdency on ASCII.

UPDATE Oh, and as pointed out in a comment, you can't change the value of s and then pass the changed value to free(), that is undefined behavior also. The address passed to free() must be the same as the one you got back from malloc().

Use a separate variable for the iteration:

char *p = s;

and only free() the original s.

Answer 2

Firstly, by s=(char*)malloc(sizeof(char)); you are allocating only 1 byte of memory for buffer. Allocate enough memory to store the input. Also avoid typecasting malloc() result. Better version looks like

s = malloc(MAX * sizeof(*s));/* MAX is num of bytes you need to define */

Secondly don't use gets() use fgets() instead. Read man 3 gets or check https://linux.die.net/man/3/gets

Finally use int main(void) { } instead of just main(){ }

And more importately when you do free(s) at that time s doesn't point to memory which was earlier allocated to it because of s++ so it may result error like

free(): invalid pointer:

So don't change s use s[row] while iterating OR you can assign s to other pointer and then you can do free(s).

Complete code

int main() {
        char *s = NULL;
        int size = MAX*sizeof(*s);/*define MAX value, it is no of bytes need*/
        s = malloc(size);/* this is generic
                            sizeof(*s) works for any data type */
        printf("Enter a string:\n");
        fgets(s,size,stdin);/* use fgets() instead of gets() */
        int row = 0;
        while (s[row]!='\0') {
                if ( *(s+row) >= 65 && *(s+row) <= 90)
                        printf("%c",*(s+row) + 32);
                else if( *(s+row) >=97 && *(s+row) <= 122)
                        printf("%c",*(s+row) - 32);
                else
                        printf("%c",*(s+row));
                row++;
        }
        free(s);/* s still points to same location */
        return 0;
}

Also you can use isupper() instead of comparing each char ASCII value.

Answer 3

This is wrong.

s = (char*)malloc(sizeof(char));
printf("Enter a string:\n");
gets(s);

s = (char*)malloc(sizeof(char)); allocates 1 byte of memory. And then with gets(s); you get a string, which will be Undefined Behavior.

You have to changed it to

s = malloc(MAX_LENGTH * sizeof(char));   //MAX_LENGTH is user defined

Additionally, you must check if malloc() returned anything. If it returns NULL then it means no memory is allocated and all of the existing program will invoke undefined behavior. Also, there is no need to cast the malloc result, so to further improve your code, you need to change it to,

s = malloc(MAX_LENGTH * sizeof(char));
if(s == NULL)
{
    // Add error handling here
}

Also,

void main()

isn't by the standard anymore, see This post which explains why. If you want to know what C11 standard states about it, then refer the standard here: Section 5.1.2.2.1 Program startup

So change it to,

int main(void)

You should make sure that you call free(s); only if it was allocated. As one of the comments below rightly indicates that free(NULL); is NOT a problem, but it also have no effect, so why call it anyway.

Make s point to NULL again, but its irrelevant in this piece of code.