Skip to content

Commit

Permalink
switch to double lock checking for windows eventlog writer
Browse files Browse the repository at this point in the history
  • Loading branch information
phuslu committed Jul 5, 2024
1 parent 212b9f9 commit ba29440
Showing 1 changed file with 61 additions and 33 deletions.
94 changes: 61 additions & 33 deletions eventlog.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
"unsafe"
)

// EventlogWriter is an Writer that writes logs to windows event log.
// EventlogWriter is a Writer that writes logs to windows event log.
type EventlogWriter struct {
// Event Source, must not be empty
Source string
Expand All @@ -20,56 +20,79 @@ type EventlogWriter struct {
// Event Host, optional
Host string

once sync.Once
register *syscall.LazyProc
deregister *syscall.LazyProc
report *syscall.LazyProc
handle uintptr
mu sync.Mutex
handle uintptr
}

var (
advapi32 = syscall.NewLazyDLL("advapi32.dll")
procRegisterEventSourceW = advapi32.NewProc("RegisterEventSourceW")
procDeregisterEventSource = advapi32.NewProc("DeregisterEventSource")
procReportEventW = advapi32.NewProc("ReportEventW")
)

// Write implements io.Closer.
func (w *EventlogWriter) Close() (err error) {
var ret uintptr
ret, _, err = syscall.Syscall(w.deregister.Addr(), 1, w.handle, 0, 0)
ret, _, err = syscall.Syscall(procDeregisterEventSource.Addr(), 1, w.handle, 0, 0)
if ret > 0 {
err = nil
}
return
}

// WriteEntry implements Writer.
func (w *EventlogWriter) WriteEntry(e *Entry) (n int, err error) {
w.once.Do(func() {
if w.ID == 0 {
err = errors.New("Specify eventlog default id")
return
}

if w.Source == "" {
err = errors.New("Specify eventlog source")
return
}
func (w *EventlogWriter) connect() (err error) {
if w.handle != 0 {
w.Close()
w.handle = 0
}

var s *uint16
if w.Host != "" {
s = syscall.StringToUTF16Ptr(w.Host)
}
if w.ID == 0 {
err = errors.New("Specify eventlog default id")
return
}

advapi32 := syscall.NewLazyDLL("advapi32.dll")
w.register = advapi32.NewProc("RegisterEventSourceW")
w.deregister = advapi32.NewProc("DeregisterEventSource")
w.report = advapi32.NewProc("ReportEventW")
if w.Source == "" {
err = errors.New("Specify eventlog source")
return
}

w.handle, _, err = syscall.Syscall(w.register.Addr(), 2, uintptr(unsafe.Pointer(s)), uintptr(unsafe.Pointer(syscall.StringToUTF16Ptr(w.Source))), 0)
if w.handle != 0 {
err = nil
var host *uint16
if w.Host != "" {
host, err = syscall.UTF16PtrFromString(w.Host)
if err != nil {
return
}
})
}

var source *uint16
source, err = syscall.UTF16PtrFromString(w.Source)
if err != nil {
return
}

w.handle, _, err = syscall.Syscall(procRegisterEventSourceW.Addr(), 2, uintptr(unsafe.Pointer(host)), uintptr(unsafe.Pointer(source)), 0)
if w.handle != 0 {
err = nil
}

return
}

// WriteEntry implements Writer.
func (w *EventlogWriter) WriteEntry(e *Entry) (n int, err error) {
if w.handle == 0 {

This comment has been minimized.

Copy link
@xuyang2

xuyang2 Jul 5, 2024

Contributor

syscall 包的 LazyDLL/LazyProc 在加锁之前的 if ... == nil 用的也是 Race-detector-friendly version
golang/go@9b4aaa4
https://golang.org/cl/6856046

还有 sync.Once.Do() 的第一个判断也是用的 Race-detector-friendly version
https://github.com/golang/go/blob/go1.16/src/sync/once.go#L57
https://github.com/golang/go/blob/go1.16/src/sync/once.go#L67

This comment has been minimized.

Copy link
@xuyang2

xuyang2 Jul 5, 2024

Contributor

github copilot:

在有双重检查锁定(double-check locking)的情况下,将第一个检查条件从 atomic.LoadPointer((*unsafe.Pointer)(unsafe.Pointer(&d.dll))) == nil 改为简单的 if d.dll == nil 是不安全的。原因如下:

  1. 内存可见性和重排序atomic.LoadPointer 不仅保证了原子性,还保证了内存的可见性。在多线程环境中,一个线程对共享变量的修改,通过原子操作可以立即对其他线程可见。而简单的 if d.dll == nil 检查不具备这样的内存可见性保证。此外,编译器或CPU可能会对操作进行重排序,破坏了双重检查锁定的安全性。

  2. 原子性保证atomic.LoadPointer 保证了操作的原子性,即在执行过程中不会被其他线程的操作中断。而直接访问 d.dll 可能会在读取其值的同时被其他线程修改,导致不一致的状态。

  3. 双重检查锁定的正确实现:双重检查锁定模式的正确实现依赖于对共享变量访问的原子性和内存可见性保证。第一次检查避免了每次访问都需要加锁的开销,而第二次检查(在锁内部)确保了只有一个线程可以进行初始化。去掉原子操作,就无法保证这一点。

因此,即使存在双重检查锁定,也不应该将 atomic.LoadPointer 的调用替换为简单的 if d.dll == nil 检查。这样的改变会破坏代码的线程安全性,可能导致竞态条件和其他并发问题。

This comment has been minimized.

Copy link
@phuslu

phuslu Jul 5, 2024

Author Owner

这类问题我刚好前不久研究过,结论如下:
https://x.com/phuslu/status/1782634032884593099
https://x.com/phuslu/status/1782635198930358747
不过对于初始化的场景,的确用atomic提高了跨平台的安全性,我接下来也改改相关代码,谢谢!
对于性能敏感的场景,比如推文提到 Logger.Level 读取,我还是保留现在的方式。

w.mu.Lock()
if w.handle == 0 {
err = w.connect()
if err != nil {
w.mu.Unlock()
return
}
}
w.mu.Unlock()
}

const (
EVENTLOG_SUCCESS = 0x0000
EVENTLOG_ERROR_TYPE = 0x0001
Expand Down Expand Up @@ -101,10 +124,15 @@ func (w *EventlogWriter) WriteEntry(e *Entry) (n int, err error) {

var ecat uintptr = 0
var eid = w.ID
var ss = []*uint16{syscall.StringToUTF16Ptr(b2s(e.buf))}
var ss = []*uint16{nil}

ss[0], err = syscall.UTF16PtrFromString(b2s(e.buf))
if err != nil {
return
}

var ret uintptr
ret, _, err = syscall.Syscall9(w.report.Addr(), 9, w.handle, uintptr(etype), ecat, eid, 0, 1, 0, uintptr(unsafe.Pointer(&ss[0])), 0)
ret, _, err = syscall.Syscall9(procReportEventW.Addr(), 9, w.handle, uintptr(etype), ecat, eid, 0, 1, 0, uintptr(unsafe.Pointer(&ss[0])), 0)
if ret > 0 {
err = nil
}
Expand Down

0 comments on commit ba29440

Please sign in to comment.