hectane / go-acl

Go library for manipulating ACLs on Windows
MIT License
120 stars 30 forks source link

Error code propagation from api incorrect #16

Closed jazzy-crane closed 1 year ago

jazzy-crane commented 3 years ago

I noticed when I tried to set ACLs on a mispelled folder I got an error back which when printed said The operation completed successfully. (i.e. ERROR_SUCCESS)

Looking at the API package:

func GetNamedSecurityInfo(objectName string, objectType int32, secInfo uint32, owner, group **windows.SID, dacl, sacl, secDesc *windows.Handle) error {
    ret, _, err := procGetNamedSecurityInfoW.Call(
        uintptr(unsafe.Pointer(windows.StringToUTF16Ptr(objectName))),
        uintptr(objectType),
        uintptr(secInfo),
        uintptr(unsafe.Pointer(owner)),
        uintptr(unsafe.Pointer(group)),
        uintptr(unsafe.Pointer(dacl)),
        uintptr(unsafe.Pointer(sacl)),
        uintptr(unsafe.Pointer(secDesc)),
    )
    if ret != 0 {
        return err
    }
    return nil
}

The 3rd parameter of the syscall-type function should only be used for functions which return a TRUE/FALSE and vend their error through the GetLastError() mechanism. In the case of GetNamedSecurityInfoW the error code is what is returned. Therefore the code should look more like:

func GetNamedSecurityInfo(objectName string, objectType int32, secInfo uint32, owner, group **windows.SID, dacl, sacl, secDesc *windows.Handle) error {
    ret, _, _ := procGetNamedSecurityInfoW.Call(
        uintptr(unsafe.Pointer(windows.StringToUTF16Ptr(objectName))),
        uintptr(objectType),
        uintptr(secInfo),
        uintptr(unsafe.Pointer(owner)),
        uintptr(unsafe.Pointer(group)),
        uintptr(unsafe.Pointer(dacl)),
        uintptr(unsafe.Pointer(sacl)),
        uintptr(unsafe.Pointer(secDesc)),
    )
    if ret != 0 {
        return windows.Errno(ret)
    }
    return nil
}

I haven't looked at the api package more exhaustively to see if this applies to other functions, but I expect it does. If I find time I will submit an MR with a small test and hopefully fixes for these (though as a larger fix it may be worth moving to the x/sys/windows/mkwinsyscall mechanism for generating these functions?)

nathan-osman commented 1 year ago

Fixed with pull request #17.