Do not leak file descriptors in backup_crontab #184
Conversation
src/crontab.c
Outdated
| FILE *crontab_file = NULL; | ||
| FILE *backup_file = NULL; |
There was a problem hiding this comment.
The initialization should not be necessary as we never call fclose with these FILE * variables uninitialized. Also calling fclose on NULL is afaik not allowed so please check whether this isn't the case.
There are missing fclose calls on lines 566 and 593 though, could you please add them?
There was a problem hiding this comment.
Also calling fclose on NULL is afaik not allowed so please check whether this isn't the case.
TIL; I assumed that passing NULL resumed in no-op, but this is apparently not the case according to standard. Which also means that the cleanup section is wrong – if opening of the backup file fails, fclose(backup_file) is called with backup_file == NULL. I'll add guard checks for that.
The initialization should not be necessary as we never call fclose with these FILE * variables uninitialized.
True; I'll drop them then.
There are missing fclose calls on lines 566 and 593 though, could you please add them?
I'll take a look.
There was a problem hiding this comment.
There are missing fclose calls on lines 566 and 593 though, could you please add them?
Well, the exit() calls there kill the process at that point; any open files will be closed by the OS. Are the calls really necessary?
There was a problem hiding this comment.
Probably not critically but then the same thing can be said about the other missing fclose() calls being added by this PR as the crontab process is short lived so missing some fcloses() on error cases would do no real harm.
src/crontab.c
Outdated
| printf("Backup of %s's previous crontab saved to %s\n", User, backup_path); | ||
|
|
||
| return 0; | ||
| cleanup: | ||
| if (backup_file) (void) fclose(backup_file); |
There was a problem hiding this comment.
Please use backup_file != NULL and put the statement on a separate line.
Originally, if anything went wrong during the backup, the early return caused the crontab_file and possibly backup_file pointers to leak. Issue found by static scanner.
|
Updated with the requested changes. |
|
Merged. Thank you for your contribution. |
Originally, if anything went wrong during the backup, the early return caused the crontab_file and possibly backup_file pointers to leak.
Issue found by static scanner.