Opening files using fopen only if user enters correct password

Question

I wrote this program to just get some practice in C, but I have a question:

1. Do you see anything that I am doing that will create bad habits in my future programming?

#include <stdio.h>

char password[]="ps"; // set the password that user must match
char input[20];          // user input buffer
char filename[32];
FILE *fp;
int c,i,count=3;

int main()
{

printf("Please enter the password:");   //ask for password
fgets(input,20,stdin);                  //store user entry into input buffer
sscanf(input,"%s",input);               //format

    if(strcmp(password,input)==0)   //if the input is equal to the password then open the file and print contents
    {
            if((fp=fopen("text","r")) != NULL)
            {
                    while((c=getc(fp)) != EOF)
                    {
                            putchar(c);
                    }
                    fclose(fp);                     //close the file once done
            }
            else
            {
                    perror("Error:");
                    return(-1);
            }
    }
    else if(strcmp(password,input) != 0)    //if the input is wrong then give the user 3 more tries
         {
                    for(i=0;i<3;i++)
                    {
                            printf("\nThe password you entered is incorrect!");
                            printf("\n%d tries left, password:",count);
                            fgets(input,20,stdin);
                            sscanf(input,"%s",input);

                            --count;

                            if(strcmp(password,input)==0)   //if out of those 3 tries entry is correct--open file/print contents
                            {
                                    if((fp=fopen("text","r")) != NULL)
                                    {
                                            while((c=getc(fp)) !=EOF)
                                     {
                                                    putchar(c);
                                            }
                                            fclose(fp);     //close file
                                            break;          //break out of for loop
                                    }
                                    else
                                    {
                                            perror("Error:");
                                            return(-1);
                                    }
                            }
                    }
            }
}

Answer 1

Don't repeat yourself. This block of code appears twice. It would be better to extract it to a separate function and reuse it:

        if((fp=fopen("text","r")) != NULL)
        {
                while((c=getc(fp)) != EOF)
                {
                        putchar(c);
                }
                fclose(fp);                     //close the file once done
        }
        else
        {
                perror("Error:");
                return(-1);
        }

Btw, is there a reason to read and print the file contents character by character? It would be more efficient to do it buffer by buffer.

Also, the comment is pointless, that line is self-explanatory.

---

In this code:

if(strcmp(password,input)==0)
{
        // ...
}
else if(strcmp(password,input) != 0)

The else if condition can be replaced with a simple else. That's less typing, and again, don't repeat yourself!

Answer 2

Don't use/declare global variables, instead declare them in main and pass them to the functions that use them. This makes it more clear where they are used. If there is lots of data that you want to pass to a function create a struct with them and pass it to the function, caveat: only place things that logically belong together in a struct

Always initialize all variable declarations and try to place them on different lines, it is a bit more verbose but more clear (IMHO).

When you use fgets, use the sizeof on the buffer to avoid later changin the size of the buffer and forgetting to change all fgets where it is used.

fgets( input, sizeof(input), stdin );

Always explain any constants e.g. count=3 (no magic numbers)

Instead of lots of indent levels try to split up your program into functions e.g. you have one part where you read the text file text, that could be placed in a separate function e.g. int showFile(const char* fileName);

always looks for patterns to break out into separate functions -- another repeated functionality you have is reading the password from the keyboard, this could be placed in a separate function as well which returns the password.

e.g. char* getPassword(const char* prompt, char* passwordBuffer, int maxLen);

use puts instead of printf when there is only one argument with no format specifiers

puts("The password you entered is incorrect!");

your main function should return an integer even though some compilers add it automatically you should make a habit to add a return statement in main.

use descriptive variable names, 'c','i' are poor choices.

Answer 3

• Try to keep your indentation consistent everywhere. You indent by different number of spaces in different places, making it hard for others to read. The number of spaces is up to you, but in C, it's most common to indent by four spaces.

• Your use of whitespace (except for indentation) is okay, but this:

  for(i=0;i<3;i++)
  

  can also use more whitespace:

  for (i = 0; i < 3; i++)
  

  This makes it easier to see everything clearly at a glance.

• One fundamental thing you'll hear many people say: don't use global variables.

  char password[]="ps";
  char input[20];
  char filename[32];
  FILE *fp;
  int c,i,count=3;
  

  Why is that? Global variables can be modified anywhere within the file, making it hard to tell what has modified it thus introducing potential bugs and maintenance issues. Sure, they may be easier to use since they're available everywhere, but they can just cause more problems.

  Basically, you'll know one is in global scope when it's declared or initialized outside of a function. When using functions, you should have these variables inside of them, making them local. You'll then be able to pass them to other functions that need them. This limited scope allows you to have a better idea of what entity can modify them, particularly within that function.

• This is more of a minor thing, but is still worthwhile. Whenever you have unformatted output (not displaying any variables, only hard-coded text):

  printf("Please enter the password:");
  

  You can use a function called puts():

  puts("Please enter the password:");
  

  It still does the same thing, but also gives you an extra newline at the end.

Answer 4

And in regarding to hints of habits which could make you trouble in the future: If you use array sizes, make it via define. If you are handling with biger code, and you decide at some point: it would be better to make the buffer of size 30 instead of 20. Then you have to review the whole code for each numeric 20 regarding to that size and change it to 30. Even harder if you made some arithmetic with it not containing the 20 directly but expecting it.

So better do

#define USER_INPUT_BUFFER_SIZE 20
#define MAX_FILE_NAME_LEN 32

char input[USER_INPUT_BUFFER_SIZE];          // user input buffer
char filename[MAX_FILE_NAME_LEN];

instead of:

char input[20];          // user input buffer
char filename[32];

So when ever you do any check or operation regarding to the max size of the buffer, write USER_INPUT_BUFFER_SIZE (the compiler will replace it by 20 then).

And whenever you need to change the size, just change the value of the define. And everything is on the right place without having to lookup anything in the sourcecode.

Answer 5

One more problem with this is that, even with that 3 strikes rule, this program can still EASILY be brute-forced. There's no protection against just doing 3 tries and then rebooting the program, which can be done REALLY fast. it might take a while to crack it, but it'll work. Especially since this program is something that can easily be run in parallel (there will only be 1 instance that accesses the file at a time).

The problem is that there isn't really a good solution for this, because solutions will require external variables, which can also relatively easily be edited.