Why do I need to initialize dynamically allocated array at allocation time with ()?

Question

I have the following piece of code:

#include <cassert>
#include <cstring>
#include <iostream>

class Line
{
public:
  Line(const char* c)
  {
    len = std::strlen(c);
    text = new char[len + 1]();
    std::strncpy(text, c, len);
  }

  Line& operator=(const Line& rhs)
  {
    if (&rhs == this) return *this;

    delete[] text;
    text = new char[rhs.getLen()]();
    std::strncpy(text, rhs.getText(), rhs.getLen());
    return *this;
  }

  Line(const Line& rhs)
  {
    len = rhs.getLen();
    text = new char[rhs.getLen() + 1]();
    std::strncpy(text, rhs.getText(), len);
  }

  ~Line()
  {
    delete[] text;
  }

  unsigned getLen() const
  {
    return len;
  }

  char* getText() const
  {
    return text;
  }

private:
  char* text;
  unsigned len;
};

// operator overloads

const Line operator*(const Line& lhs, const Line& rhs)
{
  unsigned newLen = lhs.getLen() + rhs.getLen();
  char* newC = new char[newLen + 1]();
  std::strncat(newC, lhs.getText(), lhs.getLen());
  std::strncat(newC, rhs.getText(), rhs.getLen());
  Line line(newC);
  delete[] newC;
  return line;
}

bool operator==(const Line& lhs, const Line& rhs)
{
  return strcmp(lhs.getText(), rhs.getText()) == 0;
}

std::ostream& operator<<(std::ostream& os, const Line& l)
{
  os << l.getText();
  return os;
}

int main(void)
{
  Line l("hello");
  Line l2("hello");
  Line r("world");
  Line x = l * r;

  assert(strcmp("hello", l.getText()) == 0);
  assert(strcmp("helloworld", x.getText()) == 0);
  assert(l == l2);
  assert(!(r == l2));

  std::cout << l << '\n';
  std::cout << r << '\n';
  std::cout << x << '\n';
}

Without the parenthesis at line:

    text = new char[len + 1];

I will get assert at line

assert(strcmp("helloworld", x.getText()) == 0);

a.out: main.cpp:83: int main(): Assertion `strcmp("hello", l.getText()) == 0' failed.

And without the asserts I will get a sanitizer error:

==29167==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200000eff6 at pc 0x7f0f48d679cb bp 0x7fffb3df3820 sp 0x7fffb3df2fd0
READ of size 7 at 0x60200000eff6 thread T0
    #0 0x7f0f48d679ca in __interceptor_strlen (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x6d9ca)
    #1 0x7f0f48a9f938 in std::basic_ostream<char, std::char_traits<char> >& std::operator<< <std::char_traits<char> >(std::basic_ostream<char, std::char_traits<char> >&, char const*) (/usr/lib/x86_64-linux-gnu/libstdc++.so.6+0xb2938)
    #2 0x40114a in operator<<(std::ostream&, Line const&) /home/XXX/programming/c/XXX/main.cpp:72
    #3 0x401276 in main /home/XXX/programming/c/XXX/main.cpp:88
    #4 0x7f0f48432ec4 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
    #5 0x400ed8  (/home/XXX/programming/c/XXX/a.out+0x400ed8)

0x60200000eff6 is located 0 bytes to the right of 6-byte region [0x60200000eff0,0x60200000eff6)
allocated by thread T0 here:
    #0 0x7f0f48d8f30a in operator new[](unsigned long) (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x9530a)
    #1 0x4014fd in Line::Line(char const*) /home/XXX/programming/c/XXX/main.cpp:11
    #2 0x40121d in main /home/XXX/programming/c/XXX/main.cpp:78
    #3 0x7f0f48432ec4 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 __interceptor_strlen
Shadow bytes around the buggy address:
  0x0c047fff9da0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9db0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9dc0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9dd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9de0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa 00 03
=>0x0c047fff9df0: fa fa fd fd fa fa 06 fa fa fa 06 fa fa fa[06]fa
  0x0c047fff9e00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9e10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9e20: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9e30: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9e40: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
==29167==ABORTING

Why do I need to initialize this memory? (equivalent of using std::fill)

[Operator new initializes memory to zero?](http://stackoverflow.com/a/7546745/14065) — Martin York, Feb 06 '16 at 13:18

score 4 · Answer 1 · answered Feb 06 '16 at 13:16

4

You need to do it because your strncpy is too short to include the terminating zero in the original string, leaving the last element of the destination unmodified.

This also means that your program is undefined.

answered Feb 06 '16 at 13:16

molbdnilo

64,751
3
43
82

score 1 · Accepted Answer · answered Feb 06 '16 at 13:20

The problem is your usage of std::strncat. It appends a line to the end of current line. The end of current line is determined by finding a terminating 0 in current line. If you do not initialize your data, you will end with many random characters which are unlikely to include 0 as first element (therefore it will be treated as some pre-existing data and it will cause a buffer overflow).

Use strncpy/strcpy to copy first line:

unsigned newLen = lhs.getLen() + rhs.getLen();
char* newC = new char[newLen + 1];
std::strncpy(newC, lhs.getText(), lhs.getLen() + 1);
std::strncat(newC, rhs.getText(), rhs.getLen() + 1);

Why do I need to initialize dynamically allocated array at allocation time with ()?

2 Answers2