2

I apologise if this has been answered before but I have been driving myself mad searching for an answer. I have been using malloc to create an array:

char *message = (char*) malloc(31 *sizeof(char));

To get the user to enter a message of maximum 30 characters:

 fgets(message, 30, stdin);

In another function I have then tried to convert the message to lower case and store it in another string (of hopefully equal length as the message).

int length_message = strlen(message) - 1;

char *temp1 = (char*) malloc(length_message * sizeof(char));

for(int i=0; message[i] != '\0'; i++) 
{
        temp1[i] = tolower(message[i]);     
}

But when I come to display the new string

puts(temp1);

It shows my input message followed by a whole load of gibberish on a new line. I cannot understand where this is coming from.

Furthermore upon checking the length of my temp1 array and the value I created for length message, using: 

printf("length of temp array: %d", strlen(temp1));
printf("length of message: %d\n", length_message);

Which returned the desired length_message (number of characters input by user in message) but the wrong number for the length of my temp array! Have I incorrectly allocated the memory for my temporary array or have I not written to that array correctly?

From what I have read on other threads, I seem to have a buffer overflow error? How do I keep this in check?

Thanks in advance!

Some programmer dude
  • 400,186
  • 35
  • 402
  • 621
rus64
  • 77
  • 3
  • 10

2 Answers2

3

This:

strlen(message) - 1

Should of course be this:

strlen(message) + 1

You made a typo.

cutsoy
  • 10,127
  • 4
  • 40
  • 57
1

There are a couple of problems with your code.

The actual error is caused by the fact that you don't copy the terminating 0 character from the source string to the destination. You need to write a temp[i] = 0; after the for loop.

Also, you do indeed have a possibility of buffer overrun, since you are malloc()ating one less character for the copying than the length of the string. (Why...? You should be allocating one more...)

Further problems include the horrible casting of void *, the redundant (and consequently unsafe) sizeof(char), and the hard-coded buffer size.

Instead of

char *message = (char*) malloc(31 *sizeof(char));

try

#define BUF_SIZE 31

char *message = malloc(BUF_SIZE);
fgets(message, BUF_SIZE, stdin);

or if you feel pedantic then

char *message = malloc(BUF_SIZE * sizeof message[0]);

but never use a hard-coded type in sizeof() when you are malloc()ating memory. But anyway, I don't think you need malloc() at all. Why don't you use an automatic array (a "stack-allocated" one, although that's not the correct name for it) instead? Then you could take advantage of the sizeof operator when getting user input.

Furthermore, strlen() and several other stdlib functions return size_t. You should absolutely be using that type instead of int.

Community
  • 1
  • 1
  • Thank you so much for the speedy and helpful response. What benefit does using #define BUF_SIZE have? As opposed to say: char *message = malloc(31); – rus64 Nov 29 '13 at 20:16
  • @rus64 The advantage is that you don't have two independent hard-coded sizes, thus you will less likely mess up the buffer size when passing to `malloc()` and `fgets()`. Imagine that you change the size in `malloc()` to 16, but you forget to do the same for `fgets()`... the result is horrible. –  Nov 29 '13 at 20:18
  • Ah thank you. 'You need to write a temp[i] = 0; after the for loop.' Not sure exactly what this will give me? – rus64 Nov 29 '13 at 20:27
  • @rus64 A NUL-terminator. –  Nov 29 '13 at 20:31