0

I am have the below snippet of code which allocates the memory out of the bounds:

char *str1 = (char *) malloc(sizeof(char) * BUF_SIZE);
printf ("str1 = ");
scanf("%s", &str1);
int n = strlen(str1);

Initially I was getting a Segmentation Fault in strlen(). After playing with gdb I came to know that str1 was at a address which was out of bounds. Below shown is the gdb output.

(gdb) print str1
$1 = 0x636261 <Address 0x636261 out of bounds>

Note: The breakpoint was set on the line where strlen() is called. Also, BUF_SIZE is set as #define BUF_SIZE 10

Any help would be greatly appreciated. Thanks :)

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
Siddharth Kamaria
  • 2,448
  • 2
  • 17
  • 37
  • Add `memset(str,0,BUFSIZE)` before 'reading' into it – ForceBru Jan 27 '15 at 15:12
  • Don't use `scanf` use `fscanf` or `fgets` instead. – Algo Jan 27 '15 at 15:14
  • @Algo: Why? I usually advise against `*scanf()` myself, but in this case that's hardly the issue, especially if suggesting `fscanf()` instead, which has just the same issues as `scanf()`...? – DevSolar Jan 27 '15 at 15:16

2 Answers2

8

You should be passing str1, not &str1, to scanf().

scanf() expects a char * for the "%s" format; you pass the address of a char *. This does not lead to happiness.

Since BUF_SIZE is so small — just 10, you say — you need to use:

if (scanf("%9s", str1) != 1)
    …process error or EOF…

This will protect you against buffer overflow. You should specify the size every time you use %s (unless you use the POSIX modifier %ms to scanf(), but then the rules all change). If you don't, scanf() can write outside the bounds of your string variable without knowing.

You should also check that malloc() succeeds. Always. Every time.

Note that compiling with GCC and -Wall (or -Wformat) will point out the error of your ways. If you're using GCC, you should always compile with -Wall (and preferably -Wextra too — I use more options than that) to get better error reporting.

For a file with your code in it, GCC said:

warning: format ‘%s’ expects argument of type ‘char *’, but argument 2 has type ‘char **’ [-Wformat=]

or 'error' when compiling with -Werror too, which I regard as good practice.

I note in passing that GDB is telling me that you probably entered abc as the string on a little-endian (e.g. Intel) machine. The value 0x636261 corresponds to that. You overwrote the pointer returned by malloc() because you passed the address of str1 instead of the value in str1 — leaving memory corruption.

Jonathan Leffler
  • 730,956
  • 141
  • 904
  • 1,278
4

You have several problems in your code:

  1. Don't read a string over the pointer, you meant scanf("%s", str1);, drop the &!
  2. Don't cast the return value of malloc() in C.
  3. Don't assume that allocations succeed.
  4. Don't use scanf() for this, it's very dangerous since you're not passing it any information about the available buffer size. It's better to use fgets().
Community
  • 1
  • 1
unwind
  • 391,730
  • 64
  • 469
  • 606