BUG: memory where pointer pointing to changed accidently

[william]

My question is: Specific memory block where my pointer pointing to
changed strangely, seemingly that no statement changed it.

Here are two examples I got:
***********1*****************
I was about to read from a floppy image and build a tree for all the
directories and files.
My question is only about a small portion where I had debugging
problem, and I marked the place below at two places using
"<======================"(you can try to run the code)

Can anyone point out why newPtr->content(name, size, etc) changed
after return from
contructNode()
*****************************

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

struct treeNode
{
char* name;
int size;
int isDir;
int disk_pos;
struct treeNode *leftPtr;
struct treeNode *rightPtr;
};

typedef struct treeNode TreeNode;
typedef TreeNode * TreeNodePtr;

void create_tree(TreeNodePtr *,int, FILE*);
void insert(TreeNodePtr *, TreeNodePtr);
TreeNodePtr construct_Node(unsigned char*);
TreeNodePtr chg_dir(TreeNodePtr *, char*);
void disp_dir(TreeNodePtr);

int main(int argc, char** argv)
{
TreeNodePtr rootPtr=NULL;
TreeNodePtr current_dir=NULL;
TreeNodePtr parent_dir=NULL;
TreeNodePtr cursor=NULL;

char **dirs;
char* delimitors="/";
char* token;
FILE * pFile;
int i=0;
int depth;

//setup the position of cursor, parent_dir, and current_dir;
current_dir=rootPtr;
cursor=current_dir;

pFile=fopen("/home/william/Downloads/DOS5.IMA","rb");
if(pFile==NULL)
{
printf("File can not be opened.\n");
}
else
create_tree(&rootPtr, 19, pFile);
fclose(pFile);

/*
if(argv[0]=="cd")
{
if(argv[1]!="." && argv[1]!=".." && argc>=2)
{
token=strtok(argv[1],delimitors);
while (token !=NULL)
{
dirs=token;
i++;
token=strtok(NULL,delimitors);
}
for (depth=0;depth<i;depth++)
{
cursor=chg_dir(&cursor,dirs[depth]);
parent_dir=current_dir;
current_dir=cursor;
}
}
// else if(argv[1]=="..")
}
else if (argv[0]=="lsfd")
{
disp_dir(current_dir);
}
*/
}

void disp_dir(TreeNodePtr treePtr)
{
if(treePtr !=NULL)
{
printf("%s\n",treePtr->name);
printf("%d\n",treePtr->size);
printf("%d\n",treePtr->isDir);

disp_dir(treePtr->leftPtr);
disp_dir(treePtr->rightPtr);
}
}

TreeNodePtr chg_dir(TreeNodePtr *startPtr, char * dir_name)
{

if((*startPtr)->name==dir_name)
{
return *startPtr;
}
else if (strcmp((*startPtr)->name,dir_name))
chg_dir(&((*startPtr)->rightPtr),dir_name);
else if (strcmp(dir_name,(*startPtr)->name))
chg_dir(&((*startPtr)->leftPtr),dir_name);


}
void create_tree(TreeNodePtr *treePtr, int sect_num, FILE *pFile)
{
TreeNodePtr newPtr;
unsigned char item[32];
char bits=0x00;;
char mask=0x01;
int i;
int entry_num=0;
int counter=0;

fseek(pFile,sect_num*512+entry_num*32,SEEK_SET);
entry_num++;
fread(item,32,1,pFile);
newPtr=construct_Node(item);
do
{
insert(treePtr,newPtr);
//if the current item is a directory,create tree for it
if(newPtr->size==0)
create_tree(treePtr, newPtr->disk_pos,pFile);
//read next item and construct new node
fseek(pFile,sect_num*512+entry_num*32,SEEK_SET);
fread(item,32,1,pFile);
newPtr=construct_Node(item);<===============returned, problem
happened
//check whether this item valid by testing whether the name is
printable
for (i=0;i<8;i++)
{
if(isprint((newPtr->name)) )
bits=bits|(mask<<i);
}
bits=bits|0x00;
entry_num++;
counter++;
}while (bits);
}

void insert(TreeNodePtr *treePtr, TreeNodePtr newPtr)
{
if(*treePtr==NULL)
{
if (newPtr !=NULL)
{
*treePtr=newPtr;
printf("*treePtr=%p",*treePtr);
}

else
printf("Entry for %s can not be inserted. No more memory.\n",
newPtr->name);
}
else
{
int result=strcmp(newPtr->name,(*treePtr)->name);
if (result<0)
{
insert(&((*treePtr)->leftPtr), newPtr);
printf("%s inserted as left node.\n", newPtr->name);
}
else if (result>0)
{
insert(&((*treePtr)->rightPtr), newPtr);
printf("%s inserted as left node.\n", newPtr->name);
}
}
}

int byteArrToInt(unsigned char buffer[],int index )
{

int integer=(int)buffer[index+3];
integer=(integer<<8)+(int)buffer[index+2];
integer=(integer<<8)+(int)buffer[index+1];
integer=(integer<<8)+(int)buffer[index];
return integer;
}

TreeNodePtr construct_Node(unsigned char *item)
{
char name[8];
int i,size=0,disk_pos=0,isDir=0;
//construct entry name;
for (i=0;i<8;i++)
{
name=item;
}
//retrieve entry size
size=byteArrToInt(item,28);
//retrieve cluster number
disk_pos=(((int)item[27])<<8)+(int)item[26];
//decide if this entry is for directory or not
if(size==0)
{
isDir=1;
}
//create new node using the information above
TreeNodePtr newPtr=malloc(sizeof(TreeNode));
if (newPtr !=NULL)
{
newPtr->name=name;
newPtr->size=size;
newPtr->isDir=isDir;
newPtr->disk_pos=disk_pos;
newPtr->leftPtr=NULL;
newPtr->rightPtr=NULL;
return newPtr;//<=======================================
//<===here, when the function returns, the
content of newPtr->anymember
//changed!
}
else
{
return NULL;
}
}

**********2*************************
almost the same phenomina, memory changed accidently
I used <=================== to mark up the bugs as above,
It was interesting: if I only input one parameter, it worked fine; if
I input two parameters, it doesn't work (you will know what I am
saying if you run the code, it will hint you to input parameters)
***********************************
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "fs.h"

void instructions(void);
void getArgs(char**,char**);

int main()
{
char* cmd=NULL, *args=NULL;
int i;
char cmd_img[]="fdimg";


getArgs(&cmd,&args);//<==================no need to go further, I
was stucked here
if(strlen(cmd)==0)
printf("\n/> ");
else if(strcmp(cmd,cmd_img)==0)
{
if(args!=NULL)
// create_img(args);
else
printf("Please specify the (path/)name of the image.");
}
else printf("Invalid command. ");
getArgs(&cmd,&args);

return 0;
}

void getArgs(char **cmdPtr, char **argsPtr)
{
char input[256];
char *token;
char *arguments[2]={NULL,NULL};
int i=0,index=1;

for(i=0;i<256;i++)
input=0x0;
printf("\n/> ");
fgets(input,256,stdin);
token=strtok(input," \n");
*cmdPtr=token;
for(i=0;i<10;i++)
printf("%X",*(*cmdPtr+i));
printf("just after the *cmdPtr=token;\n");
while(token!=NULL)
{
token=strtok(NULL, " \n");
arguments[index]=token;
index++;
}
*argsPtr=arguments[1];//<========================
}
void instructions()
{
printf("Please type one of the following commands:\n");
printf("fdimg [path][filename] to create an image of the floppy.
\n");
printf("dispart to display the partition information of all hard
drives.\n");
printf("lsfd to list the content of current directory and of
subdirectory of current directory.\n");
printf("dpfile to display the content of a file in a floppy
diskette.\n");
printf("exit to exit this program.\n");
}

 

[Jens Thoms Toerring]

My question is: Specific memory block where my pointer pointing to
changed strangely, seemingly that no statement changed it.

Here are two examples I got:
***********1*****************
I was about to read from a floppy image and build a tree for all the
directories and files.
My question is only about a small portion where I had debugging
problem, and I marked the place below at two places using
"<======================"(you can try to run the code)

Can anyone point out why newPtr->content(name, size, etc) changed
after return from
contructNode()
*****************************

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

struct treeNode
{
char* name;
int size;
int isDir;
int disk_pos;
struct treeNode *leftPtr;
struct treeNode *rightPtr;
};

typedef struct treeNode TreeNode;
typedef TreeNode * TreeNodePtr;

A lot of people recommend aginst using typedefs, especially when
they hide the "pointerness" of a variable...

void create_tree(TreeNodePtr *,int, FILE*);
void insert(TreeNodePtr *, TreeNodePtr);
TreeNodePtr construct_Node(unsigned char*);
TreeNodePtr chg_dir(TreeNodePtr *, char*);
void disp_dir(TreeNodePtr);

int main(int argc, char** argv)
{
TreeNodePtr rootPtr=NULL;
TreeNodePtr current_dir=NULL;
TreeNodePtr parent_dir=NULL;
TreeNodePtr cursor=NULL;

char **dirs;
char* delimitors="/";
char* token;
FILE * pFile;
int i=0;
int depth;

//setup the position of cursor, parent_dir, and current_dir;
current_dir=rootPtr;

Useless since already done via the initialization.

cursor=current_dir;

Same here.

pFile=fopen("/home/william/Downloads/DOS5.IMA","rb");
if(pFile==NULL)
{
printf("File can not be opened.\n");
}
else
create_tree(&rootPtr, 19, pFile);
fclose(pFile);

You should try to close 'pFile' only when fopen() did succeed.

void create_tree(TreeNodePtr *treePtr, int sect_num, FILE *pFile)
{
TreeNodePtr newPtr;
unsigned char item[32];
char bits=0x00;;
char mask=0x01;
int i;
int entry_num=0;
int counter=0;

fseek(pFile,sect_num*512+entry_num*32,SEEK_SET);

You need to check if fseek() did succeed.

entry_num++;
fread(item,32,1,pFile);

You definitely should test if fread() did succeed.

newPtr=construct_Node(item);

int byteArrToInt(unsigned char buffer[],int index )
{
int integer=(int)buffer[index+3];

You better make that an unsigned int and make sure that sizeof(int)
is really 4 on the system you are trying to run that on.

integer=(integer<<8)+(int)buffer[index+2];
integer=(integer<<8)+(int)buffer[index+1];
integer=(integer<<8)+(int)buffer[index];
return integer;
}

TreeNodePtr construct_Node(unsigned char *item)
{
char name[8];
int i,size=0,disk_pos=0,isDir=0;
//construct entry name;
for (i=0;i<8;i++)
{
name=item;
}

Wouldn't make a memcpy() more sense here? And I hope you realize
that you not necessarily have now a string, i.e. something ter-
minated by a '\0' character.

//retrieve entry size
size=byteArrToInt(item,28);
//retrieve cluster number
disk_pos=(((int)item[27])<<8)+(int)item[26];
//decide if this entry is for directory or not
if(size==0)
{
isDir=1;
}
//create new node using the information above
TreeNodePtr newPtr=malloc(sizeof(TreeNode));

Do you have a C99 compiler or are you compiling that as C++?
Otherwise you will have to define the 'newPtr' variable at
the start of the function.

if (newPtr !=NULL)
{
newPtr->name=name;

Here's someting that's going to drop on your feed: you assign to
the name member a pointer to a variable that is local to this
function. And thus this it will point to some memory you don't
own anymore the moment you leave the function. You need allocated
memory instead here.

**********2*************************
almost the same phenomina, memory changed accidently
I used <=================== to mark up the bugs as above,
It was interesting: if I only input one parameter, it worked fine; if
I input two parameters, it doesn't work (you will know what I am
saying if you run the code, it will hint you to input parameters)
***********************************
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "fs.h"

void instructions(void);
void getArgs(char**,char**);

int main()

Better make that

int main( void )

since your main(() function doesn't take arguments.

{
char* cmd=NULL, *args=NULL;
int i;
char cmd_img[]="fdimg";

getArgs(&cmd,&args);//<==================no need to go further, I
was stucked here

void getArgs(char **cmdPtr, char **argsPtr)
{
char input[256];
char *token;
char *arguments[2]={NULL,NULL};
int i=0,index=1;

for(i=0;i<256;i++)
input=0x0;

If you really think that's necessary better use memset().

printf("\n/> ");
fgets(input,256,stdin);

Better test the return value of fgets()

token=strtok(input," \n");

Splitting on '\n' in the string you read in with fgets() doesn't
make real sense since fgets() stops the moment it finds a '\n',
so tere will only be one at most and always at the end.

*cmdPtr=token;

Now 'cmdPtr' points to the start of the 'input' array that is
local to this function. Thus, once you left the function, the
memory it points to will be reused for other purposes and you
can't use this pointer anymore. You need to copy the 'token'
string to allocated memory and make 'cmdPtr' point to that.

for(i=0;i<10;i++)
printf("%X",*(*cmdPtr+i));

What's that supposed to do? And using '%X' to print a char isn't
a good idea.

printf("just after the *cmdPtr=token;\n");
while(token!=NULL)
{
token=strtok(NULL, " \n");
arguments[index]=token;
index++;

'arguments' has only 2 elements (and you only start with using
the second one since 'index' is initialized to 1)). So the second
round through this loop you start writing past the end of the
'arguments' arrray.

}
*argsPtr=arguments[1];//<========================

Same problem as above: 'argPtr' now points somewhere into the
innards of the 'index' array which stops to exist the moment
you leave the function.

Regards, Jens

 

[Richard Heathfield]

Jens Thoms Toerring said:

A lot of people recommend aginst using typedefs,

But not everyone.

especially when they hide the "pointerness" of a variable...

Ah, that's different. Yes, everyone recommends aginst that. (Well,
everyone worth listening to...)

 

[Nick Keighley]

Jens Thoms Toerring said:

But not everyone.


Ah, that's different. Yes, everyone recommends aginst that. (Well,
everyone worth listening to...)

I've got 750 kloc that does it all the time. Am I not a lucky person?

 

[Richard Heathfield]

Nick Keighley said:

Am I not a lucky person?

Today is not your day. Tomorrow isn't looking good either. You might
want to stay in bed this week.

The same advice applies to all those born under the sign of
Stradivarius, or whatever it's called.

 

[Servé Laurijssen]

I've got 750 kloc that does it all the time. Am I not a lucky person?

Meaning you wrote that yourself and are therefore pro typedefing pointers or
you have to support that code and wanna kill yourself now?

 

[william]

Meaning you wrote that yourself and are therefore pro typedefing pointers or
you have to support that code and wanna kill yourself now?

Thank you Jens, for your patience to read my code and all the
comments. I appreciate that a lot.

Does it mean that once I return from some function, I lost control of
the local variables and memories within the scope the function?

Or can I can make it clearer in this way: Pointer is better supposed
to point to someplace legal in memory?

Thank you

 

[Jens Thoms Toerring]

Does it mean that once I return from some function, I lost control of
the local variables and memories within the scope the function?

If you leave a function all automatic (in contrast to static)
local variables vanish and you if you pass back a pointer to
one of them you end up with holding a pointer that points to
memory that you don't own anymore and that contains more or
less random data.

So if you want to pass back a pointer from a function it must
point to memory you still own after the end of the function.
You typically do that by calling malloc() and return back the
pointer value you got from that function. E.g.

#include <stdio.h>
include <stdlib.h>

struct A {
char *good;
char *bad;
};

void init_A( struct A *a )
{
char b[ ] = "Can only used within this function";

if ( ( a->good = malloc( 100 ) ) == NULL )
exit( EXIT_FAILURE );
strcpy( a->x, "test data" );
a->bad = b;
printf( "bad is %s\n", a->bad );
}

int main( void )
{
struct A a;

init_A( &a );
printf( "good is %s\n", a.good );
free( a.good );
return EXIT_SUCCESS;
}

In this case you can use 'a.good' also in the caller since the
memory it points to was obtained by malloc() and you own it until
you call free() on the pointer (which you shouldn't forget, other-
wise you would create a memory leak).

In contrast what 'a.bad' points to can only used in init_A(), once
you have left the function it's forbidden to use that pointer since
the memory it points to has "gone back to the system" for re-use,
e.g. for local automatic variables of the next function you call.
If you try it anyway you invoke undefined behavior and all kinds
of strange things can happen, including that it looks for some
time as if everything would be ok.

Regards, Jens