On Tue, Jun 08, 2021 at 03:40:00PM +0200, Lénaïc Huard wrote:
> +static int systemd_timer_write_unit_templates(const char *exec_path)
> +{
> + char *filename;
> + FILE *file;
> + const char *unit;
> +
> + filename = xdg_config_home_systemd("git-maintenance@.timer");
> + if (safe_create_leading_directories(filename)) {
> + error(_("failed to create directories for '%s'"), filename);
> + goto error;
> + }
> + file = fopen_or_warn(filename, "w");
> + if (file == NULL)
> + goto error;
> + FREE_AND_NULL(filename);
Here we free the filename variable. But later...
> + unit = "# This file was created and is maintained by Git.\n"
> + "# Any edits made in this file might be replaced in the future\n"
> + "# by a Git command.\n"
> + "\n"
> + "[Unit]\n"
> + "Description=Optimize Git repositories data\n"
> + "\n"
> + "[Timer]\n"
> + "OnCalendar=%i\n"
> + "Persistent=true\n"
> + "\n"
> + "[Install]\n"
> + "WantedBy=timers.target\n";
> + if (fputs(unit, file) == EOF) {
> + error(_("failed to write to '%s'"), filename);
> + fclose(file);
> + goto error;
> + }
> + if (fclose(file) == EOF) {
> + error_errno(_("failed to flush '%s'"), filename);
> + goto error;
> + }
If we see an error we'll try to use it as part of the message. I think
the FREE_AND_NULL() can just be moved down here. And really just be
free(), since we then immediately reassign it:
> + filename = xdg_config_home_systemd("git-maintenance@.service");
> + file = fopen_or_warn(filename, "w");
> + if (file == NULL)
> + goto error;
> + FREE_AND_NULL(filename);
And then this one has the same problem. We free here, but...
> + unit = "# This file was created and is maintained by Git.\n"
> + "# Any edits made in this file might be replaced in the future\n"
> + "# by a Git command.\n"
> + "\n"
> + "[Unit]\n"
> + "Description=Optimize Git repositories data\n"
> + "\n"
> + "[Service]\n"
> + "Type=oneshot\n"
> + "ExecStart=\"%s/git\" --exec-path=\"%s\" for-each-repo --config=maintenance.repo maintenance run --schedule=%%i\n"
> + "LockPersonality=yes\n"
> + "MemoryDenyWriteExecute=yes\n"
> + "NoNewPrivileges=yes\n"
> + "RestrictAddressFamilies=AF_UNIX AF_INET AF_INET6\n"
> + "RestrictNamespaces=yes\n"
> + "RestrictRealtime=yes\n"
> + "RestrictSUIDSGID=yes\n"
> + "SystemCallArchitectures=native\n"
> + "SystemCallFilter=@system-service\n";
> + if (fprintf(file, unit, exec_path, exec_path) < 0) {
> + error(_("failed to write to '%s'"), filename);
> + fclose(file);
> + goto error;
> + }
> + if (fclose(file) == EOF) {
> + error_errno(_("failed to flush '%s'"), filename);
> + goto error;
> + }
...use it in the error messages. This one could also just be free()
before we return:
> + return 0;
> +
> +error:
> + free(filename);
> + systemd_timer_delete_unit_templates();
> + return -1;
> +}
And all of the jumps to the error label are fine, since it frees
the filename (and we don't have to worry about FREE_AND_NULL, since it
would always be valid during those jumps).
-Peff
