fix: Attempt to create log file if it does not exist. #1322
Conversation
…ile we want a file.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@micahsnyder tagging you cause this might have been forgotten. Managed to get the CI all green and put it in review. Just a reminder. |
| return 0; | ||
| } | ||
| int ret = 0, field_no = 1; | ||
| char *current_path, *file_path = cli_safer_strdup(fcConfig->logFile), *token; |
There was a problem hiding this comment.
These pointers should be initialized to NULL.
The strdup call should also be on a separate line and should have error handling.
| int ret = 0, field_no = 1; | ||
| char *current_path, *file_path = cli_safer_strdup(fcConfig->logFile), *token; | ||
| FILE *logg_fp = NULL; | ||
| token = cli_strtok(file_path, field_no++, PATHSEP); |
There was a problem hiding this comment.
I think it would be easier to follow if this line were moved down to just above the while() loop.
On the topic of readability, Some blank lines between unrelated lines of code would be nice on the eyes.
| current_path = (char *)malloc(2); | ||
| strcpy(current_path, PATHSEP); | ||
| STATBUF sb; | ||
| while (token != NULL) { | ||
| current_path = (char *)realloc(current_path, strlen(current_path) + strlen(token) + 2); | ||
| strcat(current_path, token); | ||
| free(token); | ||
| token = cli_strtok(file_path, field_no++, PATHSEP); | ||
| if (token == NULL) { | ||
| break; | ||
| } | ||
| if (LSTAT(current_path, &sb) == -1) { | ||
| if (mkdir(current_path, 0755) == -1) { | ||
| printf("ERROR: Failed to create required directory %s. Will continue without writing in %s.\n", current_path, fcConfig->logFile); | ||
| ret = -1; | ||
| goto cleanup; | ||
| } | ||
| #ifndef _WIN32 | ||
| if (current_user->pw_uid != db_owner->pw_uid) { | ||
| if (lchown(current_path, db_owner->pw_uid, db_owner->pw_gid) == -1) { | ||
| printf("ERROR: Failed to change owner of %s to %s. Will continue without writing in %s.\n", current_path, fcConfig->dbOwner, fcConfig->logFile); | ||
| ret = -1; | ||
| goto cleanup; | ||
| } | ||
| } | ||
| #endif /* _WIN32 */ | ||
| } | ||
| strcat(current_path, PATHSEP); | ||
| } | ||
| if ((logg_fp = fopen(fcConfig->logFile, "at")) == NULL) { | ||
| printf("ERROR: Can't open %s in append mode (check permissions!).\n", fcConfig->logFile); | ||
| ret = -1; | ||
| goto cleanup; | ||
| } | ||
| #ifndef _WIN32 | ||
| lchown(fcConfig->logFile, db_owner->pw_uid, db_owner->pw_gid); | ||
| #endif /* _WIN32 */ |
There was a problem hiding this comment.
Some inline comments on this process would be nice.
My understanding is it basically tries to do a mkdir -p on the logfile directory path and change ownership to the database owner as needed. Once all the directories are confirmed to exist and be owned by the right user, it will open a log file and change ownership as needed.
The logic seems good to me for linux/unix systems. I'll have to try it out.
I see an effort to compile on Windows but I don't think it'll work on Windows. Paths on windows start with a drive letter, unless it's a relative path. Since this is called for all systems, I expect it to fail to run on Windows, or create some extra directories somewhere (maybe in the current directory?).
This change is also going to require that the freshclam.conf UpdateLogFile option is always an absolute path. I suspect it works okay as a relative path right now so this could possibly break someone's configuration.
Fixes: #1155
It might seem a bit messy but the gist of the PR is that.
fc_upsert_logg_filemethod, which attempts to create the directories & log file as required.drop_privilegescase/branch later during the execution. If we do this before wefc_upsert_logg_fileour log file, then we can't userootprivileges even iffreshclamis started asroot.chownthe directories (and file, overkill?) we introduce the fielddbOwnerin thefc_configstructure.Seems this has been handled in documentation as a step of the setup.