a little c programming that no works!!

[Vinicio Flores]

Hello C hackers, this program that I am writing print "Segmentation
fault" when I execute it, I think that the problem is the gets
function, but I am not sure

/** A simple database system **/

#include "datbase.h"

static int search_and_open_data_base(char *Arg)
{
FILE *f;

f = fopen(Arg, "r");

if(!f)
return 1;
else
return 0;
}

/* ************************************************ */

static FILE *create_database(char *arg)
{
FILE *newbase;

newbase = fopen(arg, "w+");

return newbase;
}

/* ************************************************ */

static int print_options()
{
clrscr();
setattr(REVERSE);
setcolor(FOREBLUE);

gotoxy(2, 2);
cprintf("DatBASE");

gotoxy(2, 4);
setattr(NORMAL);
setcolor(FOREGREEN);
cprintf("I)Insert new register R)Print a register S) Exit");

gotoxy(2, 6);
setcolor(FORERED);
cprintf("Option: ");

setattr(NORMAL);
normvideo();

}

/* ************************************************ */

static int open_and_fill_database(char *arg)
{
FILE *database;

struct datbase_registers *base = malloc ( sizeof(struct registers
*));

database = fopen(arg, "ra");

gotoy(10);
puts("Insert register name: ");
gets(base->regname);

gotoy(11);
puts("Insert name: ");
gets(base->name);

gotoy(12);
puts("Insert telephone number: ");
scanf("%ld", base->ntel);

gotoy(13);
puts("Insert age: ");
scanf("%ld", &base->age);

fprintf(database, "Register name: ");
fputs(base->regname, database);

fprintf(database, "\n\nPerson's name: ");
fputs(base->name, database);

fprintf(database, "\nTelephone number: %lf\n", base->ntel);

fclose(database);
}

/* ************************************************ */

static int get_and_process_input()
{
int ch;

ch = getch();

switch(ch)
{
case 'i':
case 'I':
open_and_fill_database("data.base");
break;
case 'r':
case 'R':
break;
case 's':
case 'S':
setattr(NORMAL);
normvideo();
reset();
ret;
break;
default:
gotoy(9);
cprintf("?Unknown operation\n");
sleep(1);
print_options();
get_and_process_input();
break;
}
}

/* ********************|*************************** */
/* --------------------|--------------------------- */
/* ************| MAIN FUNCTION |******************* */

int main()
{
FILE *database;

int c;
int search = search_and_open_data_base("data.base");


if(search == 1)
create_database("data.base");

else
{
print_options();
get_and_process_input();
}
return 0;
}


Here is the datbase.h C header:
/** A simple database system **/

#include <ttk.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#define MAX ((900*800)*256)

struct datbase_registers
{
char regname[MAX];
char name[MAX];
long int ntel;
long int age;
};

 

[Ian Collins]

Hello C hackers, this program that I am writing print "Segmentation
fault" when I execute it, I think that the problem is the gets
function, but I am not sure

Don't use gets(), use fgets().

static int open_and_fill_database(char *arg)
{
FILE *database;

struct datbase_registers *base = malloc ( sizeof(struct registers
*));

You should check to see if this succeeds!

#define MAX ((900*800)*256)

Are you serious? 184 million!

struct datbase_registers
{
char regname[MAX];
char name[MAX];
long int ntel;
long int age;
};

So one of these is > 400MB!

 

[BartC]

Hello C hackers, this program that I am writing print "Segmentation
fault" when I execute it, I think that the problem is the gets
function, but I am not sure

/** A simple database system **/

#include "datbase.h"

static int search_and_open_data_base(char *Arg)
{
FILE *f;

f = fopen(Arg, "r");

if(!f)
return 1;
else
return 0;
}

Since you can't access the file after returning, you might as well close it
first:

fclose(f);
return 1;

since re-opening later could cause problems.

 

[Dirk Zabel]

Am 27.06.2011 06:07, schrieb Ian Collins:

Don't use gets(), use fgets().



You should check to see if this succeeds!

of course, you should get enough memory for a struct registers variable
instead of a struct registers POINTER variable!
i.e.
struct database_registers *base =
malloc(sizeof(struct database_registers));
Good luck!

#define MAX ((900*800)*256)

Are you serious? 184 million!

struct datbase_registers
{
char regname[MAX];
char name[MAX];
long int ntel;
long int age;
};

So one of these is > 400MB!

 

[Ian Collins]

Am 27.06.2011 06:07, schrieb Ian Collins:
of course, you should get enough memory for a struct registers variable
instead of a struct registers POINTER variable!
i.e.
struct database_registers *base =
malloc(sizeof(struct database_registers));

Good spot, I was fooled by the line-wrap.

The problem wouldn't have arisen if he'd written

struct datbase_registers *base = malloc( sizeof *base );

Which is why people here recommend that form.

 

[Barry Schwarz]

Hello C hackers, this program that I am writing print "Segmentation
fault" when I execute it, I think that the problem is the gets
function, but I am not sure

Your functions do not play nicely together. You have too many calls
to fopen() and the modes do not match the intended use.

/** A simple database system **/

#include "datbase.h"

static int search_and_open_data_base(char *Arg)

This function does not do any searching.

{
FILE *f;

Don't you need to #include stdio.h for FILE and its associated
functions such as fopen?

f = fopen(Arg, "r");

if(!f)
return 1;
else
return 0;

Now you have an open file and no FILE* that points to it. You can
never close it. (This is a possible memory leak depending on how your
system allocates files.) On some system, a second attempt to open the
file for output will fail because it is already open for input.

}

/* ************************************************ */

static FILE *create_database(char *arg)
{
FILE *newbase;

newbase = fopen(arg, "w+");

return newbase;
}

/* ************************************************ */

static int print_options()
{
clrscr();
setattr(REVERSE);
setcolor(FOREBLUE);

gotoxy(2, 2);
cprintf("DatBASE");

That is a strange use of camel case.

You use a lot of non-standard functions (possibly POSIX) but you don't
seem to have the correct headers for them. In any event, this limits
the number of people who can attempt to help to just those who have
the same or a similar system. If that is essential to understanding
the problem, you might be better off posting in a group that deals
with that system.

In this case, why use a non-standard function which does nothing
different than a standard function?

gotoxy(2, 4);
setattr(NORMAL);
setcolor(FOREGREEN);
cprintf("I)Insert new register R)Print a register S) Exit");

Most menus seem to use P for Print and X for eXit.

gotoxy(2, 6);
setcolor(FORERED);
cprintf("Option: ");

setattr(NORMAL);
normvideo();

}

/* ************************************************ */

static int open_and_fill_database(char *arg)
{
FILE *database;

struct datbase_registers *base = malloc ( sizeof(struct registers
*));

Others have pointed out the error here.

database = fopen(arg, "ra");

ra is not a standard mode. Read and append makes no sense.

gotoy(10);
puts("Insert register name: ");
gets(base->regname);

Perhaps the only time no one will ever mention gets() overrunning the
target array. When you fix the size of your arrays, fgets() will be a
better choice.

gotoy(11);
puts("Insert name: ");
gets(base->name);

gotoy(12);
puts("Insert telephone number: ");
scanf("%ld", base->ntel);

Storing a telephone number as an integer can produce misleading
results if phone numbers start with 0 (as in Europe and Japan).

gotoy(13);
puts("Insert age: ");
scanf("%ld", &base->age);

You really expect age to exceed 65K?

fprintf(database, "Register name: ");
fputs(base->regname, database);

If you are going to bear the expense of calling fprintf, why not
include everything in one function call
fprintf(database, "Register name: %s\n", base->regname);
and eliminate the call to fputs?

fprintf(database, "\n\nPerson's name: ");
fputs(base->name, database);

fprintf(database, "\nTelephone number: %lf\n", base->ntel);

Here you invoke undefined behavior. ntel is an integer, not a
floating point. Did you mean %ld?

fclose(database);

You never stored age in database.

}

/* ************************************************ */

static int get_and_process_input()
{
int ch;

ch = getch();

The last input could have been age. scanf() would have left the '\n'
from the ENTER key sitting in the input buffer. That is the character
that this getch() would pick up. Not what you want.

switch(ch)
{
case 'i':
case 'I':
open_and_fill_database("data.base");
break;
case 'r':
case 'R':
break;
case 's':
case 'S':
setattr(NORMAL);
normvideo();
reset();
ret;

What kind of statement is this? Is it supposed to be a function call:
ret();

break;
default:
gotoy(9);
cprintf("?Unknown operation\n");
sleep(1);
print_options();
get_and_process_input();

Did you really mean to be recursive? Wouldn't a while loop be much
easier to debug?

break;
}

More undefined behavior. This function is defined to return an int.
You don't. Didn't your compiler generate a diagnostic for this? If
not, maybe you should up the warning level.

}

/* ********************|*************************** */
/* --------------------|--------------------------- */
/* ************| MAIN FUNCTION |******************* */

int main()
{
FILE *database;

You really need to use this variable somewhere.

int c;
int search = search_and_open_data_base("data.base");


if(search == 1)
create_database("data.base");

You created the file but threw away the FILE* the function returned.
You now have an empty file opened for output that you have no way to
process or close.

else
{

At this point, the file is open for input.

print_options();
get_and_process_input();

This may call open_and_fill_database() which will always attempt to
open data.base even if you change the file name later. This is one
occasion where having pointer to (or array containing) the file name
at file scope would be not be unreasonable, especially if it were
const.

}
return 0;
}


Here is the datbase.h C header:
/** A simple database system **/

#include <ttk.h>
#include <unistd.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#define MAX ((900*800)*256)

What benefit do you perceive from the extraneous parentheses?

struct datbase_registers
{
char regname[MAX];
char name[MAX];
long int ntel;
long int age;
};

 

[Heinrich Wolf]

....

static FILE *create_database(char *arg)
{
FILE *newbase;

newbase = fopen(arg, "w+");

return newbase;
}

....
newbase resides on the stack in the scope of create_database. After you
leave create_database, the memory space allocated for newbase is lost! You
return the pointer newbase to the caller of create_database, but after you
have left create_database, it points to freed memory. If the caller uses
this pointer, this leads to a segmentation fault!

static FILE *newbase;
will solve that problem.

kind regards
Heiner

 

[Lew Pitcher]

...
...
newbase resides on the stack in the scope of create_database.

True, but the /pointer/ stored in newbase (on successful operation of
fopen() ) points to a value outside the scope of create_database().

After you
leave create_database, the memory space allocated for newbase is lost!

True that newbase is no longer referencable after create_database() ends,
but the pointer stored in newbase doesn't point to anything within
create_database()'s scope. Instead, the /pointer/ points to something
specifically outside the scope of create_database(), and that something
does not "go away" when create_database() terminates. Thus, the /pointer/
is still valid, even after the termination of create_database().

You
return the pointer newbase to the caller of create_database, but after you
have left create_database, it points to freed memory.

No, it doesn't.

If the caller uses
this pointer, this leads to a segmentation fault!

No it won't.

 

[Barry Schwarz]

...
...
newbase resides on the stack in the scope of create_database. After you
leave create_database, the memory space allocated for newbase is lost! You
return the pointer newbase to the caller of create_database, but after you
have left create_database, it points to freed memory. If the caller uses
this pointer, this leads to a segmentation fault!

Not at all. While it is true that newbase ceases to exist when the
function returns, the memory it points to does not. In fact, the
memory it points to was determined by fopen and if that memory were to
be freed automatically at any point, it would be at the end of fopen.

static FILE *newbase;
will solve that problem.

Completely unnecessary since there is no need for newbase to continue
to exist. Its value is what is returned and that value remains valid
until a call to fclose.

 

[Keith Thompson]

...
...
newbase resides on the stack in the scope of create_database. After you
leave create_database, the memory space allocated for newbase is lost! You
return the pointer newbase to the caller of create_database, but after you
have left create_database, it points to freed memory. If the caller uses
this pointer, this leads to a segmentation fault!

static FILE *newbase;
will solve that problem.

Returning the value of a local object returns *a copy* of that value.
It's not necessary for the object to continue to exist after that. It's
as true for pointer objects as for, say, int objects.

int func(void)
{
int local = 42;
return local;
}

Here, "local" no longer exists, but func1() has no problem returning the
value 42.

It's returning the *address* of a local variable that can cause problems.

 

[Heinrich Wolf]

...
newbase resides on the stack in the scope of create_database. After you
leave create_database, the memory space allocated for newbase is lost! You
return the pointer newbase to the caller of create_database, but after you
have left create_database, it points to freed memory. If the caller uses
this pointer, this leads to a segmentation fault!

static FILE *newbase;
will solve that problem.

kind regards
Heiner

I'm sorry!

I mixed it up with someting like this:

char *SomeFunction(void)
{
char Buffer[10];
strcpy(Buffer, "Hello");
return Buffer;
}

Here you need
static char Buffer[10];

 

[Keith Thompson]

...
newbase resides on the stack in the scope of create_database. After you
leave create_database, the memory space allocated for newbase is lost! You
return the pointer newbase to the caller of create_database, but after you
have left create_database, it points to freed memory. If the caller uses
this pointer, this leads to a segmentation fault!

static FILE *newbase;
will solve that problem.

I'm sorry!

I mixed it up with someting like this:

char *SomeFunction(void)
{
char Buffer[10];
strcpy(Buffer, "Hello");
return Buffer;
}

Here you need
static char Buffer[10];

Or one of several other possibilities, such as having the caller pass
in a pointer to an array that will hold the result, or allocating
the result with malloc() and making the caller responsible for
free()ing it.

C's special-case semantics here lay a trap for the unwary. The
statement "return Buffer;" *looks* like it's innocuously returning
the value of a local object, but the implicit array-to-pointer
conversion means it's really returning the address of a local objec.