I am building a Password Manager Program in C for a school assignment and at the moment of execute the following function, the program crashes.
I first call it on this case statement.
case 1:
// new record adding
s = Inputdata();
fwrite(&s, sizeof(password), 1, f);
sA[d] = s;
d ;
break;
then we got the function Inputdata() which will storage the string of the password
password Inputdata()
{
password s;
fflush(stdin); // clearing memory buffer
printf("\nEnter name of the site for the password (Ex. Gmail password)\t:\t");
gets(s.name);
printf("\nEnter unique password identifier. \t:\t");
scanf("%d", &s.ID);
printf("\nEnter password to create\t:\t");
gets(s.pw);
return s;
}
Here is the struct for the password
struct password
{
// Data Members
char name[30];
int ID; // password ID
char pw[30]; //storage of password
};
typedef struct password password;
Why is the program crashing, I'm not the best programmer at C, I come from Python and JavaScript so if I'm not parsing the data correctly I think this may be the thing.
Also here is the full code for the program, for references:
// password record Management using file /array/function
#include <stdio.h>
#include <string.h>
#include <stdlib.h>
#include <conio.h>
// structure of password details composite structure by using mark structure
struct password
{
// Data Members
char name[30];
int ID; // password ID
char pw[30]; //storage of password
};
typedef struct password password;
// input block
password Inputdata()
{
password s;
fflush(stdin); // clearing memory buffer
printf("\nEnter name of the site for the password (Ex. Gmail password)\t:\t");
gets(s.name);
printf("\nEnter unique password identifier. \t:\t");
scanf("%d", &s.ID);
printf("\nEnter password to create\t:\t");
gets(s.pw);
return s;
}
// output block
void showdata(password s)
{
printf("\n%s\t\t%d\t%s", s.name, s.ID, s.pw);
}
// find a password according to IDs
int findpass(password s, int i)
{
if (s.ID == i)
{
showdata(s);
return 1;
}
else
{
return 0;
}
}
// menu displaying
int menu()
{
int choice;
system("cls"); // for clearing screen
printf("\n|Password Manager - C2000 v1.0|\n");
printf("1. Add new password \n");
printf("2. Delete password \n");
printf("3. Find password\n");
printf("4. Show all the password \n");
printf("7. Showing Sorted IDs\n");
printf("0. Quit\n");
printf("Pick one : ");
scanf("%d", &choice);
return choice;
}
// deleting a record from array
void delData(password *s, int cur, int di)
{
int found = 0, i, j;
for (i = 0; i < cur; i )
{
found = findpass(s[i], di);
if (found == 1)
break; // break statement outside the switch and used in loop
}
if (found == 0)
printf("No Record Found");
else
for (j = i; j < cur - 1; j )
{
s[j] = s[j 1];
}
}
password s; // global variable
// main driver program
int main()
{
// first of all making binary file
FILE *f = fopen("password1.dat", "wb "); // file oprn
int id, pw, i = 0, j;
int d;
if (!f)
{
printf("Cannot open file!\n");
return 1;
}
printf("|Password Manager - C2000 v1.0|\n ");
printf("\nPress any key to start...\n");
// loading array from binary file
password *sA = (password *)malloc(d * sizeof(password)); // dynamic memory initialization of array
fseek(f, 0, SEEK_SET);
password k, temp;
for (i = 0; i < d; i )
{
fread((char *)&sA[i], sizeof(s), 1, f); // loading array from file
showdata(sA[i]); // showing array
}
getch();
// loop will execute until not exit
while (1)
{
int ch = menu();
switch (ch)
{
case 1:
// new record adding
s = Inputdata();
fwrite(&s, sizeof(password), 1, f);
sA[d] = s;
d ;
break;
case 2: // delete from array
{
int di, found = 0;
printf("\n Enter ID to Delete\t:\t");
scanf("%d", &di);
delData(sA, d, di);
d--;
break;
}
case 3: // find password
{
int di, found = 0;
printf("\n Enter ID to Delete\t:\t");
scanf("%d", &di);
for (i = 0; i < d; i )
{
found = findpass(sA[i], di);
}
if (found == 0)
printf("No Record Found");
break;
}
case 4: // print all
for (i = 0; i < d; i )
{
fread((char *)&sA[i], sizeof(s), 1, f); // loading array from file
showdata(sA[i]);
}
getch();
break;
case 5:
// sorting of array and displaying
for (i = 0; i < d - 1; i )
{
for (j = i 1; j < d; j )
{
if (sA[i].ID > sA[j].ID) // checking and swapping
{
temp = sA[i];
sA[i] = sA[j];
sA[j] = temp;
}
}
}
// showing sorted data on screen
for (i = 0; i < d; i )
{
showdata(sA[i]);
}
break;
default:
exit(0);
}
getch();
}
free(sA);
}
CodePudding user response：
As noted in the comments there a number of issues with your program. Hopefully list this captures them all:
conio.his windows specific. Don't useget()orgets()as they are unsafe.struct password { ...; char pw[30]; }. Your struct is really an account so you end up with a name clash between the type the attribute.- (Not fixed)
struct password: name and password are fixed sized strings. It means you can create a record and write into those fields directly. On the flip side, it also means when youfwrite()the whole record you end up with a lot of padding. - Use constants instead of magic values (name length, password length, name of your password file, and version string).
Inputdata()different naming convention than all other functions.Inputdata():scanf("%d", ..)` leaves the trailing newline so you need to flush it before the next read (unless it happens to be another function that ignores leading white space.showdata(): Prefer trailing to leading newlines to ensure the output buffer is flushed.showdata(): Used to display 1 record, but 3 out 4 calls is used to display records. Make it generic to printaccounts_len` records and now it's useful in all case.findpass(): accepts a password but in both use cases it runs in a loop over all accounts. It makes sense to have it operate over all accounts and return the index of the found account and -1 for failure.findpass(): Is used multiple places so it's better that it just returns the result without writing it out.findpass(): As you return when handling the first case;else {}only adds noise. This style is called early return.menu():system("cls")is windows specific. Implement a function (like I did), or leave it out as you lose important context when clearing the screen.- (Not fixed)
menu()/main(): are tightly coupled "1" in menu matches a case inmain(). It would be better if they derive from the same data, in particular, a struct that maps between the menu selector, what is being displayed, and the function to call.menu()could also be inlined inmain(). Switching on an integer also makes it hard to read so I introduced an enum to use constants instead. - (Not fixed)
menu()/main(): 1 for add is not great user experience, it would be better to useaforadd,dfor delete etc. It's also odd that the list is semi-ordered 1 through 4, then random 7 and 0. menu():scanf("%d", ..)will leave the newline in the buffer so need to flush it in case the next i/o operation doesn't deal with leading white space.delData(): different naming convention than all other functions.delData(): It changes the size of the array so it would make sense for it to modifydi. Similar for arrays. To change a value you need to pass the address in.delData(): Instead of copying data in a loop like you do usememcpy(). It's highly optimized for this task. For instance, it might read a chunk of data and write a chunk data instead of per record. If you have a lot of data copy, however, becomes slow , if you don't care about the order of records you can overwrite the record being deleted with the last record. Another often used trick is to just mark a record as deleted. This is referred to as a tombstone. When you save your records you then just skip the tombstone. If you have lots of deletions you could also compact it once in a while. This would be referred to as a garbage collection.password s: global variables are bad, it's particular bad because it's a single character so it's hard find when you need to fix it.password s: It's the wrong type, as I think you were trying to use it for the array (sA) but it's a single struct.main(): minimize scope of variables (declare just before you need them).- (main issue)
main(): prefer to initialize variables.dis uninitialized so your whole program is undefined behavior. Even if it's 0,malloc(0)is not portable. main():fopen(..., "w")will truncate your password file.main(): check thatfis valid right after you set it.main(): consecutive print statements can be combined. In this case the name and version thingy is in your menu for the first statement can be eliminated.main(): Don't cast thevoid *frommalloc(). It is not needed.main(): You assume you havedpasswords but you have no idea how many there is. If you can change format of the file, consider writing a count of records first. Then you might be able tofread()them all in one go.main():getch()having to hit enter to us the program would array me.- (minor)
main():for(;;)is shorter thanwhile(1). main()add:sA[d] = syou cannot assign one struct to another like this. Use memcpy, or assign/strcpy each field individually.main()add: You prompt the user for an unique id, but you never check for that. It's bad user experience, program should perhaps just generate unique numbers, or don't use them at all and support find by different fields and handle more than 1 result for find.main(): Ifscanf()faildiis uninitialized so this means any record may be deleted.main()find: The prompt it wrong, it says "Delete".- `main() show: You re-read all records from the file again.
- `main() sort: You sort the data in-place which is ok but you lose the original order. If you want a sorted array insert new records in sorted order instead of at the end. If you do that, this menu option just goes away.
main()sort: Useqsort()instead of writing a worse algorithm in a case statement.- `main() exit: any wrong key and your program exit. It's better to to ignore unexpected input and only exit when needed.
- (Not fixed, minor) User interface is quite verbose with the menu being shown after every action. Consider only showing the menu when there is no password file, i.e. on first run, or run it once on start-up and then only when requested with
hor?. Besides just being verbose it obscures error messages, and data. - (Not fixed) add/delete: both manipulate size of the array. The
realloc()part could be extracted and shared so you only deal with it once. It's relatively expensive (but much less so than fwrite/fwrite) so if combined you maintain a capacity in addition to len. Then grow capacity with more than 1 record a time, and then you don't have to do it as often. This includes the initial load, you might start with allocating 100 records, and then you may not need to ever resize it. delData(): doesn't write out the new state. Probably because you found you had to rewrite parts of the file. This means those deleted records will come back from the dead if you display all records as it reloads the data from disk, or if you quit and start the program again. This a form of data corruption. It's a better design to strictly separate file i/o and add/delete.- (Not fixed) If something happens
save(), say a power outage or your device runs out of battery, your file might become corrupted. It's better write to a new file, then when that is successful move the new file over the old file. It will either work or not. If not, you have the option of doing so on start-up. Now you need a way to detect if the file is corrupted, one way is via a check sum of the data. Or leave it as a manual task to inspect the old and new files before changing anything. - (Not fixed) Error handling could be improved. For instance, by only having the user interfacing functions generate errors. Now each lower level functions need a way to communicate what failed (error codes and/or error strings etc).
- (Not fixed) Prompts are confusing. For instance when you ask for user site you give "Gmail password" as an example. It's a site or url not a password. Also it's a Google Account not a Gmail account. "Password to create", you are not really creating anything but recording the password in this system.
- (Not fixed) You may want to reconsider your choice of opening the file in binary mode. If it was just a text file, then you have the option of using an editor to make a bunch of changes and till use your program to query it
It took me hours to rewrite your program so hopefully it's helpful.
#define _XOPEN_SOURCE 500
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#define NAME_LEN 30
#define PASSWORD_LEN 30
#define PASSWORD_PATH "password1.dat"
#define VERSION "1.0"
typedef struct {
char name[NAME_LEN];
int ID;
char password[PASSWORD_LEN];
} account;
enum menu {
QUIT,
ADD,
DELETE,
FIND,
SHOW,
SORT = 7
};
int find(size_t accounts_len, const account *accounts, int id);
int add(size_t *accounts_len, account **accounts, const account *a) {
account *tmp = realloc(*accounts, (*accounts_len 1) * sizeof(**accounts));
if(!tmp) {
printf("realloc failed\n");
return -1;
}
*accounts = tmp;
memcpy(&(*accounts)[*accounts_len], a, sizeof *a);
(*accounts_len) ;
return 0;
}
void clear() {
// Move cursor to top-left corner and clear the erase entire screen
fputs("\e[;1H\e[2J", stdout);
}
int compare(const void *a, const void *b) {
int a_id = ((const account *) a)->ID;
int b_id = ((const account *) b)->ID;
if(a_id < b_id) return -1;
if(a_id == b_id) return 0;
return 1;
}
int delete(size_t *accounts_len, account **accounts, int id) {
int i = find(*accounts_len, *accounts, id);
if(i == -1)
return -1;
if(i > 0)
memmove(&(*accounts)[i], &(*accounts)[i 1], (i - 1) * sizeof(**accounts));
account *tmp = realloc(*accounts, (*accounts_len - 1) * sizeof **accounts);
if(!tmp) {
printf("realloc failed\n");
// TODO: account is gone but was not able to shrink accounts
(*accounts_len)--;
return -1;
}
*accounts = tmp;
(*accounts_len)--;
return 0;
}
int find(size_t accounts_len, const account *accounts, int id) {
for (size_t i = 0; i < accounts_len; i ) {
if(accounts[i].ID == id) {
return i;
}
}
return -1;
}
int flush() {
for(;;) {
switch(getchar()) {
case EOF: return EOF;
case '\n': return 0;
}
}
}
int load(const char *path, FILE **f, size_t *accounts_len, account **accounts) {
*accounts_len = 0;
*accounts = NULL;
*f = fopen(path, "ab ");
if (!*f) {
printf("Cannot open file!\n");
return 1;
}
fseek(*f, 0, SEEK_SET);
*accounts_len = 0;
*accounts = NULL;
for(;;) {
account a;
if(fread(&a, sizeof(a), 1, *f) != 1) {
if(feof(*f))
break;
printf("fread failed\n");
return 1;
}
if(add(accounts_len, accounts, &a) == -1) {
printf("add failed\n");
return -1;
}
}
return 0;
}
int menu() {
//clear();
printf(
"|Password Manager - C2000 v" VERSION "|\n"
"1. Add new password \n"
"2. Delete password \n"
"3. Find password\n"
"4. Show all the password \n"
"7. Showing Sorted IDs\n"
"0. Quit\n"
"Pick one : "
);
int choice = getchar();
printf("\n");
if(flush() == EOF) return EOF;
return choice - '0';
}
int save(FILE **f, const size_t accounts_len, const account *accounts) {
*f = freopen(NULL, "wb", *f);
if(!*f) {
printf("freopen failed\n");
return 1;
}
if(fwrite(accounts, sizeof *accounts, accounts_len, *f) != accounts_len) {
printf("fwrite failed\n");
return 1;
}
fclose(*f);
return 0;
}
void show(size_t accounts_len, const account *accounts) {
for(size_t i = 0; i < accounts_len; i )
printf("%s\t%d\t%s\n",
accounts[i].name,
accounts[i].ID,
accounts[i].password
);
if(accounts_len) printf("\n");
}
void strip(char *s) {
s[strcspn(s, "\n")] = '\0';
}
int main() {
FILE *f;
size_t accounts_len;
account *accounts;
if(load(PASSWORD_PATH, &f, &accounts_len, &accounts) == -1) {
printf("load failed\n");
return 1;
}
for(;;) {
switch(menu()) {
case ADD: {
account a;
printf("Enter name of the site for the password (Ex. Gmail password): ");
fgets(a.name, NAME_LEN, stdin);
strip(a.name);
printf("Enter unique password identifier: ");
if(scanf("%d", &a.ID) != 1) {
printf("scanf failed\n");
break;
}
if(flush() == EOF) return 0;
if(find(accounts_len, accounts, a.ID) != -1) {
printf("account with that id exist already\n");
break;
}
printf("Enter password to create: ");
fgets(a.password, PASSWORD_LEN, stdin);
strip(a.password);
if(add(&accounts_len, &accounts, &a) == -1) {
printf("add failed\n");
}
break;
}
case DELETE: {
int id;
printf("Enter ID: ");
if(scanf("%d", &id) != 1) {
printf("scanf failed\n");
break;
}
flush();
if(delete(&accounts_len, &accounts, id) == -1) {
printf("delete failed\n");
}
break;
}
case FIND: {
int id;
printf("Enter ID: ");
if(scanf("%d", &id) != 1) {
printf("scanf failed\n");
break;
}
flush();
int i = find(accounts_len, accounts, id);
if(i != -1) show(1, &accounts[i]);
break;
}
case SHOW:
show(accounts_len, (const account *) accounts);
break;
case SORT:
qsort(accounts, accounts_len, sizeof(*accounts), compare);
show(accounts_len, accounts);
break;
case EOF:
case QUIT:
goto out;
default:
printf("invalid menu choice\n");
break;
}
}
out:
if(save(&f, accounts_len, accounts)) {
printf("save failed\n");
return 1;
}
free(accounts);
return 0;
}
and here is example session:
|Password Manager - C2000 v1.0|
1. Add new password
2. Delete password
3. Find password
4. Show all the password
7. Showing Sorted IDs
0. Quit
Pick one : 4
|Password Manager - C2000 v1.0|
1. Add new password
2. Delete password
3. Find password
4. Show all the password
7. Showing Sorted IDs
0. Quit
Pick one : 1
Enter name of the site for the password (Ex. Gmail password): john
Enter unique password identifier: 1
Enter password to create: john
|Password Manager - C2000 v1.0|
1. Add new password
2. Delete password
3. Find password
4. Show all the password
7. Showing Sorted IDs
0. Quit
Pick one : 1
Enter name of the site for the password (Ex. Gmail password): jane
Enter unique password identifier: 0
Enter password to create: jane
|Password Manager - C2000 v1.0|
1. Add new password
2. Delete password
3. Find password
4. Show all the password
7. Showing Sorted IDs
0. Quit
Pick one : 4
john 1 john
jane 0 jane
|Password Manager - C2000 v1.0|
1. Add new password
2. Delete password
3. Find password
4. Show all the password
7. Showing Sorted IDs
0. Quit
Pick one : 7
jane 0 jane
john 1 john
|Password Manager - C2000 v1.0|
1. Add new password
2. Delete password
3. Find password
4. Show all the password
7. Showing Sorted IDs
0. Quit
Pick one : 0
|Password Manager - C2000 v1.0|
1. Add new password
2. Delete password
3. Find password
4. Show all the password
7. Showing Sorted IDs
0. Quit
Pick one : 4
jane 0 jane
john 1 john
|Password Manager - C2000 v1.0|
1. Add new password
2. Delete password
3. Find password
4. Show all the password
7. Showing Sorted IDs
0. Quit
It loads data on start-up and saves the file when the program exits. For debugging purposes it is convenient to kill the program with ctrl-c and explicit note save the file. You could save after every modification (add/delete) by moving the save() call. If you move to keeping the data sorted, your current incremental approach will no longer work.
Instead of writing your own file format seriously consider using SQLite or another database library. If you ever need change anything, say, add the record count that I mentioned above now you have to support two formats, or write a program to convert your file from one format to another. Remove ID field. Change the type of name and password to char *. Checksum at the end. Encrypt password. Write many, many programs. With SQLite those schema changes are much easier to write. You may even able to ignore fields you have deleted till you get around changing the schema. With your program you have to deal with it when you load the file.
I don't use windows so I it was not tested on your platform. Hopefully it works :-)