18

I've been asked to maintain a large C++ codebase full of memory leaks. While poking around, I found out that we have a lot of buffer overflows that lead to the leaks (how it got this bad, I don't ever want to know).

I've decided to removing the buffer overflows first, starting with the dangerous functions. What C/C++ functions that are most often used incorrectly and can lead to buffer overflow?

For compiler and/or tools used to help look for buffer overrun, I've created another question that deals with this

Community
  • 1
  • 1
MrValdez
  • 8,515
  • 10
  • 56
  • 79
  • I don't get the sense of this question. None of the functions here causes any overflow if handled with care. – unexist Oct 03 '08 at 14:43
  • 2
    @unexist: No function ever goes wrong if used correctly. The questions is what functions are most often used incorrectly and lead thus can lead to buffer overflows. – Martin York Oct 03 '08 at 14:50
  • What compiler/platform are you using? There are a lot of tools for finding this sort of thing automatically. – twk Oct 03 '08 at 15:49
  • @Martin: Indeed - so the title of the question is completely wrong. You can mark many of the C-functions with 'They may case an overflow'. – unexist Oct 03 '08 at 21:23
  • I've changed the question title. Thanks for the comments. – MrValdez Oct 04 '08 at 00:22
  • @unexist: Completely wrong. gets() cannot be "handled with care". I don't care how much memory you malloc() or how big an array you declare, I can exceed it and cause a buffer overflow. All the other unbounded char* functions suffer from similar issues. – Zathrus Oct 04 '08 at 01:07
  • @Zathrus: So malloc() always leads to an overflow? I just said that the title of the question is misleading and MrValdez has already fixed it. ;) – unexist Oct 04 '08 at 10:24

11 Answers11

13

In general, any function that does not check bounds in the arguments. A list would be

  • gets()
  • scanf()
  • strcpy()
  • strcat()

You should use size limited versions like stncpy, strncat, fgets, etc. Then be careful while giving the size limit; take into consideration the '\0' terminating the string.

Also, arrays are NOT bound checked in C or C++. The following example would cause errors. See off by one error

int foo[3];
foo[3] = WALKED_OFF_END_OF_ARRAY;

edit: Copied answers of @MrValdez , @Denton Gentry

hayalci
  • 4,089
  • 2
  • 27
  • 30
  • 2
    I disagree: functions which check bounds in the arguments are still dangerous. If you can overrun with strcpy, you can overrun with strncpy by getting the bound wrong, (e.g. increment the pointer and forget to decrement the bound). To protect yourself, use std::string or equivalent C string lib. – Steve Jessop Oct 03 '08 at 16:11
  • 1
    A shorter summary might be "the C functions risk overflows. The C++ functions generally do not" – Aaron Oct 03 '08 at 18:08
  • 5
    When using strnxxx versions, beware that they may produce a string that is not null-terminated –  Oct 03 '08 at 20:04
6

Valgrind is your new best friend.

valgrind --tool=memcheck --leak-check=full ./a.out

dicroce
  • 45,396
  • 28
  • 101
  • 140
3

The question is starting at the wrong end, I'm afraid. It's presuming that buffer overruns happen in other functions. The most common cause is operator++, in my experience, or alternatively a lack of operator!=.

The best solution to find a batch of those is /GS in Visual Studio 2005/8. It won't find all of them, but it's a cheap way to reduce the amount of manual work needed.

MSalters
  • 173,980
  • 10
  • 155
  • 350
2

Here's some functions that I found that are dangerous:

  • gets() - It doesn't check the length of the variable and can overwrite memory if the input is bigger than the variable's buffer.
  • scanf() - I'm so glad that Visual Studio told me this function is deprecated. This was an easy fix.
  • strcpy() - If the source's memory space is bigger than the destination's, the data after the destination is overwritten.
MrValdez
  • 8,515
  • 10
  • 56
  • 79
2

The following link should give you a comprehensive look at security functions in C++ (ones that are post-fixed with '_s' to fix problems like overflows): http://msdn.microsoft.com/en-us/library/8ef0s5kh(VS.80).aspx

EDIT: This link contains the specific functions that have been replaced: http://msdn.microsoft.com/en-us/library/wd3wzwts(VS.80).aspx

EDIT: I should mention these are Microsoft methods, but the link is still useful for identifying functions that were deemed a red flag.

Jordan Parmer
  • 36,042
  • 30
  • 97
  • 119
2

Unfortunately any array can result in a buffer overflow:

uint32_t foo[3];
foo[3] = WALKED_OFF_END_OF_ARRAY;

In terms of functions, sprintf will happily walk off the end of the buffer. It can be replaced by snprintf.

DGentry
  • 16,111
  • 8
  • 50
  • 66
2

Memcpy() is another dangerous one.

Any loop accessing an array is a danger point, because there's no stopping going beyond the end of array.

Memory Leaks are caused by allocating memory, and not freeing it. Constructor and destructors should be another strong review point, the latter to make sure any allocated memory is freeded.

Thomas Jones-Low
  • 7,001
  • 2
  • 32
  • 36
  • memcpy isn't that dangerous, since it accepts a length parameter, so as long as the destination buffer is in sync with the length (both of which should be under the developer's control), it doesn't matter if the source buffer is compromised. – James Curran Oct 03 '08 at 16:10
2

Which version of visual studio are you using? In 2008 with all warnings enabled, all the functions you mention (and more) warn you that they are deprecated.

Perhaps you could check that all warnings are turned on and let the compiler do the hard work for you?

As a side note, the excellent writing secure code does a great job explaining the different the pitfalls of some of the older functions.

John Sibly
  • 22,782
  • 7
  • 63
  • 80
  • I've opened another question (http://stackoverflow.com/questions/167199/what-cc-tools-can-check-for-buffer-overflows) that deals with tools for helping me look for functions that can create overflows. – MrValdez Oct 03 '08 at 14:55
2

I have somewhat the same problem on the code base I work on. My advice: be wary of any C functions that look like str*() and mem*(). Also be wary of anything that takes a pointer to a buffer, without a length. Since it seems like you have the chance to use C++ I would in the most egregious cases try to use C++ containers for things: vector, string, map, etc. These make your life a lot easier.

Also, automated problem detection tools are wonderful to have. If you can use valgrind I would recommend it. Also Rational Purify is extremely powerful, though not cheap.

Mark Kegel
  • 4,476
  • 3
  • 21
  • 21
1

An additional gotcha in C is the "strncpy()" function. Many people do not realize that it is free to return a string that is not null terminated.

jdkoftinoff
  • 2,391
  • 1
  • 17
  • 17
  • +1 [Why is `strncpy` insecure?](http://stackoverflow.com/questions/869883/why-is-strncpy-insecure). Additionally, [`strtok` is thread-unsafe](http://stackoverflow.com/questions/5999418/why-is-strtok-considered-unsafe) and risky too. – legends2k Jun 12 '14 at 14:33
0

Basically, anything which accept a pointer and writes to it, without checking the length. So thing like strcpy(), sprintf() etc.

James Curran
  • 101,701
  • 37
  • 181
  • 258