Segmentation fault (core dumped) when not using arguments

Question

I could be doing this completely the wrong way, but considering this will be for personal use, having it not be that efficient is okay.

When ran as ./todo -r, it works.

When ran as ./todo -a, it works.

When ran as ./todo, it gives me segmentation fault (core dumped)

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

int main(int argc, char *argv[]) {

    if(argc < 1) {
    printf("Not enough variables.");
    }

    if(strcmp("-r",argv[1])==0) {
        printf("\n");

        system("cat .todo");
        printf("\n");
    }
    if(strcmp("-a",argv[1])==0)   {
    char str[BUFSIZ];
    FILE *f;
    f = fopen(".todo","a");
    printf("\n\nTODO list\n\n");
    for(;;) {

        printf("~ ");
        fgets(str,256,stdin);
        if(strcmp(str,"\n")==0) {
            fclose(f);
            printf("\n");
            break;
        }

        fprintf(f,str);
        }
    return 0;
    }
}

Hint: when pasting code to SO, please remove tabs first. As you can see, indentation in the question code is not quite right... — hyde, Oct 21 '13 at 20:21

score 2 · Answer 1 · edited May 23 '17 at 12:10

2

argv[0] is the program executable name, and it's counted towards argc.

So ./todo has argc=1, but argv[1] is NULL, which will cause a problem for strcmp().

See argv[argc] ==?

Change your test:-

if (argc < 2) 
{
  printf("Not enough variables.");
  return 0; // do this, otherwise we'll plough straight on..
}

edited May 23 '17 at 12:10

Community

1
1

answered Oct 21 '13 at 20:09

Roddy

66,617
42
165
277

1

Actually, I'm 99% sure, that `argv[argc]` is required to be `NULL`, so it *is* defined... :) – hyde Oct 21 '13 at 20:13
1

@hyde is correct, but changing the check to `argc < 2` still isn't enough. See @KenWayneVanderLinde's answer. – Michael Burr Oct 21 '13 at 20:18

score 1 · Answer 2 · answered Oct 21 '13 at 20:11

1

As others have pointed out, you need argc < 2 rather than argc < 1.

Additionally, you probably want to return from the if, to stop the rest from executing:

if(argc < 2) {
  printf("Not enough variables.");
  return /* some appropriate value here */;
}

answered Oct 21 '13 at 20:11

Ken Wayne VanderLinde

18,915
3
47
72

+1 - but 'probably'? I think 'definitely', since the code right after that `if` starts accessing the `argv` array. – Michael Burr Oct 21 '13 at 20:15
@MichaelBurr: You're right that I could (fairly) confidently remove the "probably". Then again, other courses of action are possible (e.g. some default action). As it stands, his code doesn't account for this, but it's his choice what he ends up doing. – Ken Wayne VanderLinde Oct 21 '13 at 20:21

score 0 · Answer 3 · answered Oct 21 '13 at 20:09

0

You're closing your file handle and then still trying to write to it:

if (...) {
   fclose(...); <--potentially closing it, depending on the if() results
}
fprintf(...); <--potentially writing to a closed handle.

This is a BAD idea.

answered Oct 21 '13 at 20:09

Marc B

356,200
43
426
500

fprint, sorry. either way... writing to a closed handle. – Marc B Oct 21 '13 at 20:12
Shouldn't writing to a closed `FILE*` just fail cleanly with error? So not a BAD idea (such as UB), just kinda pointless/stupid (especially without checking for write errors). But I did not check docs, so I guess it could be UB or something, which is why I'm asking. – hyde Oct 21 '13 at 20:18
3

The `break` prevents writing to the closed handle, but there can still be a crash if the file open fails in the first place (resulting in `f == NULL`). – Michael Burr Oct 21 '13 at 20:22

Segmentation fault (core dumped) when not using arguments

3 Answers3