capnspacehook / taskmaster

Windows Task Scheduler Library for Go
MIT License
142 stars 29 forks source link

Calls to manage.Connect() may lead to ole not getting Uninitialized #23

Closed ItsMattL closed 3 years ago

ItsMattL commented 3 years ago

I believe there are two possible bugs in manage.Connect that may lead to ole.CoUninitialize not getting called properly.

Early error returns don't set .isInitialized or call Disconnect()

There are three error return statements after CoInitialize() has been called that will cause initialize to return. At this point, CoInitialize has been successful which means a corresponding CoUninitialize needs to happen, but it never will. Even if the caller calls Disconnect (which it doesn't appear to), isInitialized hasn't been set yet, so Uninitialize won't happen.

    err = ole.CoInitialize(0)
    if err != nil {
        return err
    }
       // Return 1
    schedClassID, err := ole.ClassIDFrom("Schedule.Service.1")
    if err != nil {
        return getTaskSchedulerError(err)
    }
       // Return 2
    taskSchedulerObj, err := ole.CreateInstance(schedClassID, nil)
    if err != nil {
        return getTaskSchedulerError(err)
    }
       // Return 3
    if taskSchedulerObj == nil {
        return errors.New("Could not create ITaskService object")
    }

err isn't checked for S_FALSE

If Connect() is called more than once, for any reason, ole.CoInitialize(0) will wind up being called multiple times. This is OK, but CoInitialize calls after the first one will return an error (S_FALSE) because ole is already initialized. This error return needs to be handled, for two reasons: One, because the init is still usable, but more importantly, because CoInitialize returning S_FALSE needs to also lead to ole.CoUninitialize(). The CoUninitialize will never happen now, because the error is returned rather than treating the connection as valid.

https://docs.microsoft.com/en-us/windows/win32/api/objbase/nf-objbase-coinitialize#return-value