5
\$\begingroup\$

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);
                                    }
                            }
                    }
            }
}
\$\endgroup\$
2
  • \$\begingroup\$ Never ever store a password as clear text. Instead store a salted hash. For example char password[]="$6$NNFkugOb88BEjABy$SAWNaGnLqUKugtIyYkq1kyn2ELFVKkcKKW594UijISAtF.G3cRh4r80dchicIPMlpAIGiudV4.d3/kDcF9w2h0"; and update the verification accordingly: if(strcmp(password,crypt(input,password))==0) \$\endgroup\$
    – kasperd
    Nov 28, 2014 at 10:25
  • \$\begingroup\$ A buffer with only 20 bytes is too short. Passwords can be longer than that. For example I always have 32 characters in my passwords. If somebody want to type a password with 100 characters in it, you should permit that as well. I have used an upper limit of 125 characters for passwords, I don't think any user would ever run into that limit, and it is still small enough that the length could be in a signed char with a couple of bytes to spare. \$\endgroup\$
    – kasperd
    Nov 28, 2014 at 10:33

5 Answers 5

7
\$\begingroup\$

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!

\$\endgroup\$
6
\$\begingroup\$

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.

\$\endgroup\$
7
  • \$\begingroup\$ regarding fgets and sizeof on the buffer: sizeof only works for buffers that are on the stack - not on the heap. \$\endgroup\$
    – Stefan
    Nov 28, 2014 at 10:52
  • \$\begingroup\$ @Stefan Is this really depending on stack and heap? As the standard doesn't mention anything about what has to be stack stored and what heap stored. And as the heap/stack allocation is a run time decision. while sizeof gets evaluated on compile time. So can it really be depending on buffer is stack/heap allocated? Or is it just a question of the buffers block scope for sizeof would work on it? I'm not pretty sure, so even as I throw in many contras, I'm interested in getting convicted by you. \$\endgroup\$
    – Zaibis
    Nov 28, 2014 at 11:37
  • \$\begingroup\$ Allocation on stack during runtime decision is, imho, not possible. char buff[CONSTANT,12345]; // stack char* buff = (char*) malloc(...); // heap The latter is, imho, not possible to find size with sizeof but this can be checked with simple program \$\endgroup\$
    – Stefan
    Nov 28, 2014 at 12:06
  • 1
    \$\begingroup\$ @Stefan Your assumption is wrong. Thats what I said. It is no where written down that malloc HAS to have allocated on the heap and array[123] on the stack. A C compiler could decide to put malloc's memory on to the stack or even a local array on to the heap. As long it keeps it handled correct (What is possible), this doesn't break any rules. And jsut because all well known compiler handle it that way (local->stack/global->heap) doesn't mean this is law. As the standard just mentions life time types and 3 diferent storage types. And how to deal with them. But not how to achieve this behaving. \$\endgroup\$
    – Zaibis
    Nov 28, 2014 at 13:15
  • 1
    \$\begingroup\$ Ok this is good to know - learned a thing today thank you. \$\endgroup\$
    – Stefan
    Nov 28, 2014 at 13:17
5
\$\begingroup\$
  • 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.

\$\endgroup\$
0
3
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ True, though I'd say judicious use of sizeof() is even better. \$\endgroup\$
    – David Z
    Nov 28, 2014 at 19:06
0
\$\begingroup\$

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.

\$\endgroup\$
1
  • 1
    \$\begingroup\$ There is no need to brute force this program. Just run it once in a debugger or disassemble the program. There is absolutely no protection granted by the algorithm. \$\endgroup\$
    – casey
    Nov 28, 2014 at 18:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.