CODEWITHSUNDEEP
c++pointers
Marcin
Marcin

Reputation: 1276

Setting a pointer to null crashes my C++ program

I have a constructor, that receives a character pointer. If it is empty, I need to set its member variable to NULL, however, the program crashes on exit when I try to.

I have verified that it gets to the line where it sets it to NULL and that is the cause of the crash.

I've tried the following:

val = NULL;

val = 0;

val = "";

Those all cause a crash, however if I used:

val = new Char[1];
val = "o";

it didn't crash.

Is there something that I'm not doing?

Update:

Here is a quick update to my problem.

The destructor I'm using is:

~LField() { 
    if (val)
      delete[] val;
}

If I take out:

if (val)
  delete[] val;

then the program doesn't crash on exit with:

val = "";

Here is some more code as requested:

LField(int rowNumVal, int colNumVal, int widthVal, const char *valVal = "", bool canEditVal = true) { 
    if(strlen(valVal) > 0) {            
      //doesn't jump in here since valVal is empty
    }
    else {
      val = ""; // this is where I'm trying to set a NULL value
    }
}

LField(const LField &clone) { 
  if (val)
    delete[] val;

  val = new char[strlen(clone.val)]; 
  strcpy(val, clone.val);
  rowNum = clone.rowNum;
  colNum = clone.colNum;
  width = clone.width;
  canEdit = clone.canEdit;
  index = clone.index;
}

LField& operator=(const LField &lfieldobj) {
    if (this != &lfieldobj) {
    if (val)
       delete[] val;

    val = new char[strlen(lfieldobj.val)];
    strcpy(val, lfieldobj.val);
    rowNum = lfieldobj.rowNum;
    colNum = lfieldobj.colNum;
    width = lfieldobj.width;
    canEdit = lfieldobj.canEdit;
    index = lfieldobj.index;
   }

   return *this;
}

Modified:

LField(int rowNumVal, int colNumVal, int widthVal, const char *valVal = NULL, bool canEditVal = true) { 
    if(valVal != NULL) {            

    }
    else {
      val = NULL; 
    }
}

LField(const LField &clone) { 
  delete[] val;
  if (clone.val != NULL) {
     val = new char[strlen(clone.val) + 1]; 
     strcpy(val, clone.val);
  }
  else
    val = NULL;
  rowNum = clone.rowNum;
  colNum = clone.colNum;
  width = clone.width;
  canEdit = clone.canEdit;
  index = clone.index;
}

LField& operator=(const LField &lfieldobj) {
    if (this != &lfieldobj) {
       delete[] val;
    if (lfieldobj.val != NULL) {                
       val = new char[strlen(lfieldobj.val) + 1];
       strcpy(val, lfieldobj.val);
    }
    else
       val = NULL;
    rowNum = lfieldobj.rowNum;
    colNum = lfieldobj.colNum;
    width = lfieldobj.width;
    canEdit = lfieldobj.canEdit;
    index = lfieldobj.index;
   }

   return *this;
}

~LField() { 
      delete[] val;
}

I've updated the code. Now val is either allocated memory with new[] or it is NULL, so there shouldn't be a problem with delete[]. However, it still crashes on exit.

Upvotes: 3

Views: 3568

Answers (5)

sharptooth
sharptooth

Reputation: 170499

In the copy constructor you try to delete[] an uninitialized pointer:

LField(const LField &clone) { 
  //good code here, then...
  if (val) //<+ some random address here
    delete[] val;//<-undefined behavior
}

just don't do that, skip the whole construct. The copy constructor is invoked on an unitilialized object, there're no resources to "free" yet.

Also you try to delete[] a string literal, that's undefined behavior. Try the following change:

LField(int rowNumVal, int colNumVal, int widthVal, const char *valVal = "", bool canEditVal = true) { 
    if(strlen(valVal) > 0) {            
      //doesn't jump in here since valVal is empty
    }
    else {
      val = new char[1];
      *val = 0;
    }
}

also the following is a buffer overrun:

val = new char[strlen(whatever)];  <-forgot to +1 for the null terminator
strcpy(val, whatever);

also checking for a null pointer before delete[] is unnecessary - delete[] on a null pointer is legal and has no effect.

Upvotes: 10

Prasoon Saurav
Prasoon Saurav

Reputation: 92864

Probably somewhere in your code you are trying to access(dereference) val which still refers to NULL.

Make sure no where in your code you are doing this

  val=NULL; //in the constructor

  //somewhere in your code      
  char ch= *val; //This would be Undefined Behavior

EDIT

You are calling delete[] on val whose value is ""(string literal), that is undefined behavior.

Some examples of UB

 1)
   char *p="hello";
  delete p; //UB
  delete []p; //UB

 2)
  char *p==new char[20]("Hello");
  delete p; //UB
  delete []p; //fine

 3) 
  char *p=new char('a');
  delete []p; //UB
  delete p; //fine

Upvotes: 3

Roddy
Roddy

Reputation: 68033

Oh dear, where to start??

a:

LField(const LField &clone) { 
  if (val)
    delete[] val;

This is daft, as val is undefined. You will be calling delete[] on random memory.

b:

  val = new char[strlen(clone.val)]; 
  strcpy(val, clone.val);

c-type Strings need a null terminator. You need to new[] and additional byte.

Upvotes: 4

codymanix
codymanix

Reputation: 29468

Calling

delete[] NULL;
delete[] 0;

is ok, you don't even need the null-check. But calling

delete[] "whatever";

is not OK since this char* wasn't allocated with new[].

Note that calling string function like strlen() like it is done in your constructor is illegal in a null reference.

You access val in your constructor before it is assigned the first time. This can also cause undefined behaviour.

Upvotes: 2

T.E.D.
T.E.D.

Reputation: 44804

I'm with Prasoon Saurav. The code you showed looks good, so the problem is somewhere else.

Try this:

  1. Change it back to val = 0;
  2. Remove all your code except the constructor, the destructor, and a declaration of an object of that class. Comment out everything else. I'd wager good repuation that it won't crash.
  3. Slowly uncomment your other code and retry the program. When it crashes again, you've found the culprit.

Upvotes: 0

Related Questions