kardianos / service

Run go programs as a service on major platforms.
zlib License
4.45k stars 678 forks source link

systemd generator may leave empty or possibly incomplete files in error cases #240

Open setharnold opened 4 years ago

setharnold commented 4 years ago

Hello, in the following section of code, there's several return err that may result in an empty file or potentially partially-written file being left on disk, but skipping the systemctl commands, etc:

https://github.com/kardianos/service/blob/a1c091bc7f8b6d1a8d28b47d8cf22068608051ef/service_systemd_linux.go#L142

    f, err := os.OpenFile(confPath, os.O_WRONLY|os.O_CREATE, 0644)
    if err != nil {
        return err
    }
    defer f.Close()

    path, err := s.execPath()
    if err != nil {
        return err
    }

    var to = &struct {
        *Config
        Path                 string
        HasOutputFileSupport bool
        ReloadSignal         string
        PIDFile              string
        LimitNOFILE          int
        Restart              string
        SuccessExitStatus    string
        LogOutput            bool
    }{
        s.Config,
        path,
        s.hasOutputFileSupport(),
        s.Option.string(optionReloadSignal, ""),
        s.Option.string(optionPIDFile, ""),
        s.Option.int(optionLimitNOFILE, optionLimitNOFILEDefault),
        s.Option.string(optionRestart, "always"),
        s.Option.string(optionSuccessExitStatus, ""),
        s.Option.bool(optionLogOutput, optionLogOutputDefault),
    }

    err = s.template().Execute(f, to)
    if err != nil {
        return err
    }

I don't know golang's templating system well -- will this perform a single atomic write to disk? Or will it perform a series of writes while executing the template?

The usual posix pattern for writing files is to create a temporary file in the correct directory with a randomly-generated name (eg mkstemp(3)), perform the operations, confirm the file is saved via checking the return value of close(2), and then perform a rename(2) operation, since that is one of the few atomic tools available.

Thanks

kardianos commented 3 years ago

There are certainly better ways of doing this. You bring up great points. But all I have time for right now is "PRs accepted" :/.