Memory issue with new[] and delete[]

Code:

---

  fseek(handle, 0, SEEK_END);
  int filesize = ftell(handle);
  rewind (handle);
  char* file_buffer = new char[filesize + 1];
  fread (file_buffer, filesize, 1, handle);
  file_buffer = strchr(file_buffer, '\n') + 1;
  file_buffer = strchr(file_buffer, '\n') + 1;
  delete[] file_buffer;

---

At that delete occurs a segmentation fault. I don't understand why, and it's increasingly agrivating as I've been dealing with this for hours and now the variable I need to be able to clear has this same issue.

it is NOT a lack of memory problem
it is NOT a lack of space assigned, I've tried everything from just the filesize to +1500, and this file is a 63byte file o_0

I highly doubt it's a problem from somewhere other than there, but here is the rest of the function at least...

Code:

---

int config::set_dest () {
  // Load the file and cut it down to the dest section
  fseek(handle, 0, SEEK_END);
  int filesize = ftell(handle);
  rewind (handle);
  char* file_buffer = new char[filesize + 1500];
  fread (file_buffer, filesize, 1, handle);
  file_buffer = strchr(file_buffer, '\n') + 1;
  file_buffer = strchr(file_buffer, '\n') + 1;
  int loop;
  char* tracker = new char[strlen(file_buffer) + 1500];
  if (strcpy(tracker, file_buffer) == NULL) {
    delete[] tracker;
    delete[] file_buffer;
    return -1;
  }
  for (loop = 1; tracker = strchr(tracker, '\n'); loop++) {
    tracker++;
    if (strchr(tracker, '\n') == NULL) {
      if (strlen(tracker) > 0)
        loop++;
      break;
    }
    if (strlen(tracker) == strlen(strchr(tracker, '\n')))
      loop--;
  }
  dest_num = loop;
  if (dest_num < 1) {
    delete[] tracker;
    delete[] file_buffer;
    return -2;
  }
  dest = new char*[dest_num];
  delete[] tracker; //Seg fault
  tracker = new char[strlen(file_buffer) - strlen(strchr(strchr(file_buffer, '\n') + 1, '\n')) + 1];
  if (strncpy(tracker, strchr(file_buffer, '\n') + 1, strlen(file_buffer) - strlen(strchr(strchr(file_buffer, '\n') + 1, '\n'))) == NULL) {
    delete[] tracker;
    delete[] file_buffer;
    return -1;
  }
  delete[] file_buffer; // Seg fault
  for (loop = 1; loop <= dest_num; loop++) {
    if (strchr(tracker, '\n') != NULL) {
      if (strlen(tracker) != strlen(strchr(tracker, '\n'))) {
        dest[loop - 1] = new char[strlen(tracker) - strlen(strchr(tracker, '\n'))];
        strncpy(dest[loop - 1], tracker, strlen(tracker) - strlen(strchr(tracker, '\n')));
        tracker = strchr(tracker, '\n') + 1;
      } else {
        tracker++;
        loop--;
        continue;
      }
    } else {
      printf ("%i", strlen(tracker));
    }
  }
  for (loop = 0; loop < dest_num; loop++)
    printf ("dest[%i]: %s\n", loop, dest[loop]);

  return 0;
}

---

Ok how about saying what the actually problem is

A segmentation fault occurs...
I wish I knew why, but I don't. -_-
That's what I'm asking... x_X

(It's a runtime issue, program compiles fine...)

el hint...

Quote:

---

$man strchr
[...]
RETURN VALUE
The strchr() and strrchr() functions return a pointer to the matched
character or NULL if the character is not found.
[...]

---

i havent looked too much at your code but id suggest using a different char* to catch the return value of strchr.

I did try that.. It didn't fix the issue...

I am only using pointers to new char[]'s for this program, could that be an issue? Using only dynamic memory for strings?

Quote:

---

Originally Posted by Zarkhalar

I did try that.. It didn't fix the issue...

I am only using pointers to new char[]'s for this program, could that be an issue? Using only dynamic memory for strings?

---

no.

try adding a null terminator to the end of your strings.

Code:

---

  char* file_buffer = new char[filesize + 1];
  fread (file_buffer, filesize, 1, handle);
  file_buffer[filesize + 1] = '\0';

  file_buffer = strchr(file_buffer, '\n') + 1; file_buffer[strlen(file_buffer) + 1] = '\0';
  file_buffer = strchr(file_buffer, '\n') + 1; file_buffer[strlen(file_buffer) + 1] = '\0';
  delete[] file_buffer;

---

Still gives me a segmentation fault.

I really don't care about any of the other code right now. I haven't even gotten to where I need this program to be, I'm so stuck on this stupid parser o_0; PHP strings are much easier to deal with :P

I just want to know why this is crashing like this? Is it because of the multiple pointers possibly, if it isn't the memory? I doubt I'd have this problem with malloc x_X

edit: Nevermind, malloc gives me the same fault at the same time -_-

Well, you're wandering off past the end of your array here:

Code:

---

file_buffer[filesize + 1] = '\0';

---

Oh, and here:

Code:

---

...; file_buffer[strlen(file_buffer) + 1] = '\0';
...; file_buffer[strlen(file_buffer) + 1] = '\0';

---

Not to mention, you're deleting the same memory twice:

Code:

---

if (strncpy(tracker, strchr(file_buffer, '\n') + 1, ...) {
    delete[] tracker;
    delete[] file_buffer;
    return -1;
  }
  delete[] file_buffer; // Seg fault

---

Fix all of that, then try again and maybe I'll read some more of your code.

[edit=doh]
Doh. I didn't see your "return -1" in there. Ignore the last one. I didn't register that line. I was thinking you called delete, then it dropped out of the IF check, and called it again.
[/edit]

Quzah.

All those things you just mentioned were mostly tests, except for the last codebox you put, which is an if statement in case something messes up in the program it gets rid of the memory first just in case...

Do the compilers consider the memory deleted instantly when it's in a if statement, or will it not delete the memory until that if statement is called?

Quzah: I agree with the out of bounds part but--

Code:

---

if (strncpy(tracker, strchr(file_buffer, '\n') + 1, ...) {
        delete[] tracker;
        delete[] file_buffer;
        return -1;
}
delete[] file_buffer; // Seg fault

---

doesn't the return statement within the body of the if statement mean that one and only one of the lines:

delete[] file_files

will ever be called? If the return statement is encountered all the code below the if statement is irrelevant and will never be seen during run-time and if the return statement isn't seen then neither can the first call to the delete operator for this memory collection because the first call to delete and the return statement are both within the same scope. Therefore, in the above snippet the memory assigned to file_buffer is gauranteed to be deleted once, and only once.

Quote:

---

Originally Posted by elad

doesn't the return statement within the body of the if statement mean that one and only one of the lines:

delete[] file_files

will ever be called?

---

Yep. See edit.

Quzah.

Again, here is the real code, fully commented

Code:

---

  fseek(handle, 0, SEEK_END); // Goes to the end possition in the file.
  int filesize = ftell(handle); // gets the length of the file.
  rewind (handle); // Goes back to the start of the file
  char* file_buffer = new char[filesize]; // a new dynamic memory variable
  fread (file_buffer, filesize, 1, handle); // Reads a small file into the new variable
  file_buffer = strchr(file_buffer, '\n') + 1; // Goes the the closest new line, then goes one point forward in the pointer to go past it so I can...
  file_buffer = strchr(file_buffer, '\n') + 1; // Go down another line, then I pass it
  delete[] file_buffer; // Now I delete the buffer. This isn't suppose to be here in the real code, but weather its at the bottom or here, this is the line that cuases the segment fault

---

This entire program is a big couple classes with variables and int functions, and the only thing the main function does is error handle the returns. It's basically a 300 line parser for a small file, but I wanted to make it more or less idiot proof ;P. There is another function right above this one that does similar stuff, no problems.. Here are both the whole functions (the set_dest one may not be completely accurate because I've been trying to fix it x_X)

Code:

---

int config::set_exempt (const char* delimiter) {
  // Load the file and cut it down to the exempt line only
  fseek(handle, 0, SEEK_END);
  int filesize = ftell(handle);
  rewind (handle);
  char* file_buffer = new char[filesize];
  fread (file_buffer, filesize, 1, handle);
  file_buffer = strchr(file_buffer, '\n') + 1;
  if (strchr(file_buffer, '\n') == NULL) {
    delete[] file_buffer;
    return -1;
  }
  if (strlen(file_buffer) == strlen(strchr(file_buffer, '\n'))) {
    delete[] file_buffer;
    return -2;
  }
  char* tracker = new char[strlen(file_buffer) - strlen(strchr(file_buffer, '\n')) + 1];
  if (strlen(file_buffer) - strlen(strchr(file_buffer, '\n')) != strlen(delimiter)) {
    strncpy (tracker, file_buffer, strlen(file_buffer) - strlen(strchr(file_buffer, '\n'))); // Keep file_buffer for later
  } else {
    delete[] file_buffer;
    delete[] tracker;
    return -3;
  }

  if (strlen(tracker) == strlen(strstr(tracker, delimiter)))
    tracker += strlen(delimiter);
  int loop;
  for (loop = 1; tracker = strstr(tracker, delimiter); loop++) {
    if (strlen(delimiter) == strlen(tracker)) {
      break;
    }
    // Try to add it so it minus's a delimiter when two are in a row :/
    tracker += strlen(delimiter);
  }
  // Reset tracker, set exempt and exempt_num
  delete[] tracker;
  tracker = new char[strlen(file_buffer) - strlen(strchr(file_buffer, '\n')) + 1];
  strncpy (tracker, file_buffer, strlen(file_buffer) - strlen(strchr(file_buffer, '\n')));
  delete[] file_buffer;
  exempt_num = loop;
  exempt = new char*[exempt_num];
  if (strlen(tracker) == strlen(strstr(tracker, delimiter)))
    tracker += strlen(delimiter);
  for (loop = 1; loop <= exempt_num; loop++) {
    if (strstr(tracker, delimiter)) {
      exempt[loop - 1] = new char[strlen(tracker) - strlen(strstr(tracker, delimiter)) + 1];
      if (strncpy (exempt[loop - 1], tracker, strlen(tracker) - strlen(strstr(tracker, delimiter))) == NULL) {
        delete[] tracker;
        delete[] exempt;
        return -4;
      }
    } else {
      exempt[loop - 1] = new char[strlen(tracker) + 1];
      if (strcpy(exempt[loop - 1], tracker) == NULL) {
        delete[] tracker;
        delete[] exempt;
        return -4;
      }
    }
    if (strstr(tracker, delimiter) != NULL) {
      tracker = strstr(tracker, delimiter);
      tracker += strlen(delimiter);
    }
  }
  delete[] tracker;
  return 0;
}

int config::set_dest () {
  // Load the file and cut it down to the dest section
  fseek(handle, 0, SEEK_END);
  int filesize = ftell(handle);
  rewind (handle);
  char* file_buffer = new char[filesize + 1];
  fread (file_buffer, filesize, 1, handle);
  file_buffer = strchr(file_buffer, '\n') + 1;
  file_buffer = strchr(file_buffer, '\n') + 1;
  delete[] file_buffer;

  int loop;
  char* tracker = new char[strlen(file_buffer) + 1500];
  if (strcpy(tracker, file_buffer) == NULL) {
    delete[] tracker;
    delete[] file_buffer;
    return -1;
  }
  for (loop = 1; tracker = strchr(tracker, '\n'); loop++) {
    tracker++;
    if (strchr(tracker, '\n') == NULL) {
      if (strlen(tracker) > 0)
        loop++;
      break;
    }
    if (strlen(tracker) == strlen(strchr(tracker, '\n')))
      loop--;
  }
  dest_num = loop;
  if (dest_num < 1) {
    delete[] tracker;
    delete[] file_buffer;
    return -2;
  }
  dest = new char*[dest_num];
  //delete[] tracker;
  tracker = new char[strlen(file_buffer) - strlen(strchr(strchr(file_buffer, '\n') + 1, '\n')) + 1];
  if (strncpy(tracker, strchr(file_buffer, '\n') + 1, strlen(file_buffer) - strlen(strchr(strchr(file_buffer, '\n') + 1, '\n'))) == NULL) {
    delete[] tracker;
    delete[] file_buffer;
    return -1;
  }
  //delete[] file_buffer;
  for (loop = 1; loop <= dest_num; loop++) {
    if (strchr(tracker, '\n') != NULL) {
      if (strlen(tracker) != strlen(strchr(tracker, '\n'))) {
        dest[loop - 1] = new char[strlen(tracker) - strlen(strchr(tracker, '\n'))];
        strncpy(dest[loop - 1], tracker, strlen(tracker) - strlen(strchr(tracker, '\n')));
        tracker = strchr(tracker, '\n') + 1;
      } else {
        tracker++;
        loop--;
        continue;
      }
    } else {
      printf ("%i", strlen(tracker));
    }
  }
 
  return 0;
}

---

How about

Code:

---

  int filesize = ftell(handle); // gets the length of the file.
  rewind (handle); // Goes back to the start of the file
  char* file_buffer = new char[filesize+1]; // a new dynamic memory variable +1 for the \0
  fread (file_buffer, filesize, 1, handle); // Reads a small file into the new variable
  file_buffer[filesize] = '\0';

---

Code:

---

  char* file_buffer = new char[filesize + 1];
  fread (file_buffer, filesize, 1, handle);
  file_buffer[filesize] = '\0';
  file_buffer = strchr(file_buffer, '\n') + 1;
  file_buffer = strchr(file_buffer, '\n') + 1;
  delete[] file_buffer;

---

Segmentation fault yet again...
I don't get the faults if I don't do two of the strchr's, but I need to get down two lines in this x_X

edit: Oddly enough, if I make the last strchr + 1 to strchr + 2, I don't get a segmentation fault...

I'm just going to rewrite the parser, prolly end up a lot smaller too x_X

Thanks for the help guys =).

You need to use a temporary buffer.

Code:

---

  char* file_buffer = new char[filesize + 1];
  char *temp_buffer;
 
  fread (file_buffer, filesize, 1, handle);
  file_buffer[filesize] = '\0';
  temp_buffer = strchr(file_buffer, '\n') + 1;
  temp_buffer = strchr(temp_buffer, '\n') + 1;
  delete[] file_buffer;

---

That's great, but now I can't delete temp.

I need to go through a file and parse it out. If I go over 26 in my increases in the pointer, I can't delete it

Soo, basically...

Code:

---

file_buffer += 26;
delete[] file_buffer; //Those work fine, but the below two don't

file_buffer += 27;
delete[] file_buffer;

---

Dynamic memory sucks :(

>That's great, but now I can't delete temp.

You shouldn't delete temp_buffer, only file_buffer. They both point to the same memory. temp_buffer just points to a location past file_buffer.

The basic rule is you need one delete for every new.

> Segmentation fault yet again...
Then your problem is somewhere else
Once you've trashed your memory, then any future use will also likely fail, no matter how correct it seems.

Err, yeah.. Whups.. Dunno why I tried to delete temp o_0;

So basically if I am playing with a dynamically allocated string, it's best to make a pointer and play with that pointer instead... ok, Thanks alot =)

>So basically if I am playing with a dynamically allocated string, it's best to make a pointer and play with that pointer instead
That's right. If you are going to be calling any function where the pointer gets changed, or incrementing the pointer, make a copy. Otherwise you've lost the address where "new" allocated it.

Now you can pass the pointer into a function, and modify it there, because a temporary copy is passed to the function. So:

Code:

---

void skip_lines(char *file_buffer)
{
  file_buffer = strchr(file_buffer, '\n') + 1;
  file_buffer = strchr(file_buffer, '\n') + 1;
  //
  // Do some processing with file_buffer here
  //
}

---

Then in main(), file_buffer would still point to the original, so:
delete []file_buffer;
would work.

Argh, Got so far and now memory comes back to haunt me. Theres a lot more to this, though.

A segmentation fault occurs in my script if there are more than 2 lines in the configuration file that occupy space (As in it's not just a new line).
I traced this segmentation fault to the following line:

Code:

---

while ((destination_directory = config.return_dest()) != NULL) {

---

Here is the code for inside the while loop:

Code:

---

while ((destination_directory = config.return_dest()) != NULL) {
      if (dirent->d_type == DT_DIR) {
        // Open up the destination file that matches this one. Start opening as a directory.
        destination_name = new char[strlen(destination_directory) + strlen(dirent->d_name) + 2];
        strcpy(destination_name, destination_directory);
        if (source[strlen(source)] != '/')
          strcat(destination_name, "/");
        strcat(destination_name, dirent->d_name);
        if ((d_directory = opendir(destination_name)) == NULL) {
          // Didn't open. Find out why and see if we can work with it!
          if (errno == ENOTDIR || ENOENT) {
            options.print_output("Directory \"",0);
            options.print_output(destination_name, 0);
            options.print_output("\" does not exist. Attempting to create...\n", 0);
            if (mkdir (destination_name, 0777) == -1) {
              options.print_output("Unable to create destination directory \"",0);
              options.print_output(destination_name, 1);
              options.print_output("\"\n", 0);
              delete[] destination_name;
              continue;
            }
            } else {
            options.print_output("Unable to read destination directory \"",0);
            options.print_output(destination_name, 1);
            options.print_output("\"\n", 0);
            delete[] destination_name;
            continue;
          }
        }
        source_name = new char[strlen(source) + strlen(dirent->d_name) + 2];
        if (source_name[strlen(source)] != '/')
          strcat(source_name, "/");
        strcat(source_name, dirent->d_name);
        copy(source_name, destination_name);
        delete[] source_name;
        delete[] destination_name;
        continue;
      }

---

Here is the code to the config.return_dest() function:

Code:

---

char* config::return_dest () {
  if (dest_loop >= dest_num) {
    dest_loop = 0;
    return NULL;
  }
  dest_loop++;
  return dest[dest_loop - 1];
}

---

I tried to figure out what was causing the segmentation fault, but couldn't see any error there... Then all of a sudden my cat jumped up on my keyboard, and killed my terminal window. So I went back in, and just ran the program from absolute path instead of just typing "bin/sync". Amazingly, no segmentation fault! Huh, lets try it from relative! Works again... So I guess I've isolated the issue to this small chunk of code, but I can't seem to get it working right:

Code:

---

        char    *env = getenv( "PATH" );
        if ( env != NULL ) {
            char buff[PATH_MAX];
            int  n;
            while ( *env &&
                    sscanf( env, "%[^:]%n", buff, &n ) == 1 ) {
                FILE *fp;
                buff[n] = '\0';
                strcat( buff, "/" );
                strcat( buff, filename );
                if ( (fp=fopen(buff,"r")) != NULL ) {

                  buff[strlen(buff) - strlen(basename(buff))] = '\0';
                  strcat (buff, "../conf/sync.conf");
                  file_id = new char[strlen(buff) + 1];
                    strcpy(file_id, buff);
                  file = file_id;
                  fclose(fp);
                  break;
                }
                env += n;
                if ( *env == ':' ) env++;
            }
      }

---

I also tried removing this particular code and setting a static value to see if it would cause a problem:

Code:

---

file = new char[strlen("./conf/sync.conf") + 1];
strcpy(file, "./conf/sync.conf");

---

No problems at all.

Thanks for the help, if you took the time to look at all that O_o;

I think you've dug such a big hole in your memory that it's impossible to tell where (and how many) problems you have.

You seem to have little idea of how strings are stored, so my guess is memory problems are rampant in your code. You always just post the bit which is falling over at the moment without really getting to the real cause of the problems. Until you grasp the idea that memory problems have "a cause" (where the bug is) and "an effect" (where it segfaults), and that these two events can be separated by large amounts of code and time, there is not a lot we can do to help.

For example
source[strlen(source)]
is always the \0 character at the end of the string, so comparing it against anything else will always fail.

And this
if (errno == ENOTDIR || ENOENT) {
is always true, because you probably meant this
if (errno == ENOTDIR || errno == ENOENT) {

Plus you seem to have generated an awful lot of code without any real testing, so any previous memory errors have gone undetected until now.


Personally, I'd suggest you
a) start again
b) use the std::string to manage all your strings for you and let it worry about all the dynamic aspects.

I do have a grasp on how memory works :/
Some parts I don't, sure. But I do create strings, I make pointers to them if I manipulate the code a lot, and I delete them when they are done.

I do find the issues by doing fprintf's to stderr for til I find the problem, then correct it....

I'm new to the memory thing, I know. The reason I was posting just bits of code is because I found how to cause a segmentation fault shortly after creating the variable, and that's how I learned that. I have no real books on this stuff, and just go from what I find. I should get a book :P

I don't think there are any memory problems in there. I always allowcate enough memory for things, if not extra o_0;

In addition to the bugs Salem pointed out, there's also this:

Code:

---

>        source_name = new char[strlen(source) + strlen(dirent->d_name) + 2];

At this point nothing has even been copied into source_name, so the if() below won't work.

>        if (source_name[strlen(source)] != '/')
>          strcat(source_name, "/");

---

Salem>Plus you seem to have generated an awful lot of code without any real testing, so any previous memory errors have gone undetected until now.

Is there a way to test the new operator? I always did if ((var = (char*) malloc(size)) == NULL) testing, but now that I'm using new I didn't think that would work.. I'll try it =)


>source[strlen(source)]
is always the \0 character at the end of the string, so comparing it against anything else will always fail.

I forgot to minus one for the array since it starts at 0 x_X thanks

Didn't even notice that swoop as well, thanks ;P

And I don't want to use the std thing... That requires iostream.h I think, eh?

I'd much rather learn how to manipulate the strings fully before using the easy way x_X