Fix Missing dll failure #1763
Fix Missing dll failure #1763
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
service/firewall/interception/dnsmonitor/etwlink_windows.go (1)
Cleanup is needed in error path to prevent resource leak
The verification confirms that
CreateStateallocates memory that must be freed byDestroySession. From the DLL source code:// PM_ETWCreateState allocates memory for the state and initializes the config for the session. PM_ETWDestroySession must be called to avoid leaks.While the finalizer helps prevent leaks in the normal path, we should explicitly clean up in the error path to ensure immediate resource release:
err := etwSession.i.InitializeSession(etwSession.state) if err != nil { + _ = etwSession.i.DestroySession(etwSession.state) return nil, fmt.Errorf("failed to initialize session: %q", err) }🔗 Analysis chain
Line range hint
44-56: Verify cleanup in error pathsWhile the finalizer helps prevent memory leaks, we should ensure proper cleanup in error paths. Currently, if
InitializeSessionfails, we might leak the state created byCreateState.Let's check for similar patterns in the codebase:
Consider adding cleanup in the error path:
err := etwSession.i.InitializeSession(etwSession.state) if err != nil { + _ = etwSession.i.DestroySession(etwSession.state) return nil, fmt.Errorf("failed to initialize session: %q", err) }🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CreateState/DestroySession pairs rg -B 5 -A 5 "CreateState|DestroySession"Length of output: 13257
service/network/connection.go (1)
553-557: Consider improving error handling logic.While the addition of error handling for
Flush()is good, the current implementation breaks the retry loop on any error. Consider whether some errors might be temporary and worth retrying.- err = module.instance.DNSMonitor().Flush() - if err != nil { - // Error flushing, dont try again. - break - } + if err = module.instance.DNSMonitor().Flush(); err != nil { + // Log the error but continue retrying + log.Tracer(pkt.Ctx()).Debugf("network: failed to flush DNS monitor: %v", err) + continue + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
service/firewall/interception/dnsmonitor/etwlink_windows.go(5 hunks)service/firewall/interception/dnsmonitor/eventlistener_windows.go(1 hunks)service/integration/etw_windows.go(1 hunks)service/integration/integration_windows.go(2 hunks)service/integration/module.go(2 hunks)service/network/connection.go(1 hunks)
🔇 Additional comments (8)
service/integration/module.go (1)
12-12: LGTM! Good improvement to event handling
The switch from StateMgr to EventMgr provides better reactive handling of initialization events, which aligns well with the PR's goal of improving DLL failure handling. The initialization in the constructor is properly done.
Also applies to: 24-26
service/integration/integration_windows.go (2)
Line range hint 44-64: LGTM! Well-structured DLL loading with proper error handling
The new loadDLL function properly:
- Finds the DLL path using platform-specific logic
- Handles loading errors with descriptive messages
- Initializes ETW functions
- Notifies listeners of successful initialization
76-79: LGTM! Type consistency improvement
The pointer return type for GetETWInterface maintains consistency with the field type change and improves null safety.
service/firewall/interception/dnsmonitor/etwlink_windows.go (2)
26-29: LGTM! Good defensive programming with nil check
The nil check for etwInterface prevents potential crashes and provides a clear error message.
71-74: LGTM! Consistent nil checks across methods
The addition of nil checks in both FlushTrace and DestroySession methods maintains consistency and prevents potential crashes.
Also applies to: 93-95
service/firewall/interception/dnsmonitor/eventlistener_windows.go (2)
27-38: LGTM! Well-structured error handling for DLL loading.
The implementation properly handles the case of missing DLL by:
- Attempting to initialize sessions
- If initialization fails, registering a callback to retry when the DLL is loaded
- Using proper error propagation
41-51: LGTM! Clean separation of concerns.
The new initializeSessions function nicely encapsulates the session initialization logic, making the code more maintainable and easier to test.
service/integration/etw_windows.go (1)
22-23: LGTM! Improved pointer semantics.
The change to return *ETWFunctions instead of value type is appropriate for struct types and maintains consistency with the rest of the codebase.
| // Try to load dll | ||
| err := i.loadDLL() | ||
| if err != nil { | ||
| log.Errorf("integration: failed to load dll: %s", err) | ||
|
|
||
| // listen for event from the updater and try to load again if any. | ||
| i.instance.Updates().EventResourcesUpdated.AddCallback("core-dll-loader", func(wc *mgr.WorkerCtx, s struct{}) (cancel bool, err error) { | ||
| err = i.loadDLL() | ||
| if err != nil { | ||
| log.Errorf("integration: failed to load dll: %s", err) | ||
| } else { | ||
| log.Info("integration: initialize successful after updater event") | ||
| } | ||
| return false, nil | ||
| }) | ||
|
|
||
| } else { | ||
| log.Info("integration: initialize successful") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Race condition concern is valid and needs synchronization
The verification confirms the race condition concern. There are multiple paths that can trigger DLL loading:
- Initial loading during
Initialize() - Callback from
EventResourcesUpdated - No synchronization mechanism exists to prevent concurrent loading attempts
This could lead to:
- Multiple simultaneous DLL loading attempts
- Race conditions when accessing
i.os.dllandi.os.etwFunctions - Potential resource leaks if a new DLL is loaded while the old one is still in use
Recommendations:
- Add a mutex to protect the DLL loading operation
- Ensure proper cleanup of existing DLL before loading a new one
- Consider adding a state check to prevent redundant loading attempts
🔗 Analysis chain
Consider potential race condition in error handling
In the error path, we register a callback that might attempt to load the DLL concurrently with other initialization attempts. Consider adding synchronization to prevent multiple simultaneous loading attempts.
Let's verify if there are other places in the codebase that might trigger concurrent loading:
Let me gather more information about potential concurrent access to the DLL loading functionality.
Let me check one more aspect to understand if there's any mutex or synchronization mechanism already in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential DLL loading triggers
rg -A 5 "loadDLL|LoadDLL"
Length of output: 2259
Script:
#!/bin/bash
# Search for initialization and usage of OSIntegration to understand concurrent access patterns
ast-grep --pattern 'type OSIntegration struct {
$$$
}'
# Also search for any calls to initialize the integration
rg -A 3 "NewOSIntegration|OSIntegration.*Initialize"
Length of output: 1564
Script:
#!/bin/bash
# Search for any synchronization mechanisms in the integration package
rg -A 3 "sync\.|Mutex|RWMutex|Lock|Unlock" service/integration/
# Also check the complete implementation of loadDLL
ast-grep --pattern 'func (i *OSIntegration) loadDLL() error {
$$$
}'
Length of output: 1699
vlabo
force-pushed
the
fix/missing-dll-failure
branch
from
4f8417d
to
ed2338f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
service/integration/integration_windows.go (1)
Line range hint
51-71: Consider additional error handling in loadDLLWhile the function handles basic errors, there are a few improvements to consider:
- The DLL release in case of ETW initialization failure
- The state cleanup if notification fails
Apply this improvement:
func (i *OSIntegration) loadDLL() error { // Find path to the dll. file, err := updates.GetPlatformFile("dll/portmaster-core.dll") if err != nil { return err } // Load the DLL. i.os.dll, err = windows.LoadDLL(file.Path()) if err != nil { return fmt.Errorf("failed to load dll: %q", err) } + + // Use defer for cleanup in case of initialization failure + var initialized bool + defer func() { + if !initialized && i.os.dll != nil { + _ = i.os.dll.Release() + i.os.dll = nil + } + }() // Enumerate all needed dll functions. i.os.etwFunctions, err = initializeETW(i.os.dll) if err != nil { return err } // Notify listeners i.OnInitializedEvent.Submit(struct{}{}) + initialized = true return nil }service/firewall/interception/dnsmonitor/etwlink_windows.go (1)
71-74: Consider consolidating duplicate nil checksThe nil check for
l.iis duplicated in bothFlushTraceandDestroySession. Consider extracting to a helper method.+func (l *ETWSession) checkInitialized() error { + if l.i == nil { + return fmt.Errorf("session not initialized") + } + return nil +} func (l *ETWSession) FlushTrace() error { - if l.i == nil { - return fmt.Errorf("session not initialized") - } + if err := l.checkInitialized(); err != nil { + return err + } // ... rest of the code func (l *ETWSession) DestroySession() error { - if l.i == nil { - return fmt.Errorf("session not initialized") - } + if err := l.checkInitialized(); err != nil { + return err + } // ... rest of the codeAlso applies to: 93-95
service/firewall/interception/dnsmonitor/eventlistener_windows.go (1)
55-57: Consistent error message styleThe error message "etw not initialized" should follow the package prefix pattern used elsewhere.
func (l *Listener) flush() error { if l.etw == nil { - return fmt.Errorf("etw not initialized") + return fmt.Errorf("dnsmonitor: ETW session not initialized") } return l.etw.FlushTrace() }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
service/firewall/interception/dnsmonitor/etwlink_windows.go(5 hunks)service/firewall/interception/dnsmonitor/eventlistener_windows.go(1 hunks)service/integration/integration_windows.go(2 hunks)service/network/connection.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- service/network/connection.go
🔇 Additional comments (3)
service/integration/integration_windows.go (2)
28-43: Synchronization improvement addresses race condition
The added mutex properly addresses the previously identified race condition concern by preventing concurrent DLL loading attempts in the callback.
83-86: Consider thread safety for GetETWInterface
The function accesses i.os.etwFunctions without synchronization. While pointer reads are atomic, consider adding RLock if the pointer can be modified concurrently.
Let's verify if there are concurrent modifications to etwFunctions:
Consider adding read lock:
func (i *OSIntegration) GetETWInterface() *ETWFunctions {
+ i.mu.RLock()
+ defer i.mu.RUnlock()
return i.os.etwFunctions
}service/firewall/interception/dnsmonitor/etwlink_windows.go (1)
26-29: Proper nil check for ETW interface
Good addition of the nil check for etwInterface parameter. This prevents potential panic and provides clear error message.
| err := initializeSessions(module, listener) | ||
| if err != nil { | ||
| return nil, err | ||
| // Listen for event if the dll has been loaded | ||
| module.instance.OSIntegration().OnInitializedEvent.AddCallback("loader-listener", func(wc *mgr.WorkerCtx, s struct{}) (cancel bool, err error) { | ||
| err = initializeSessions(module, listener) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error logging in initialization callback
The callback silently returns errors which might make debugging difficult.
module.instance.OSIntegration().OnInitializedEvent.AddCallback("loader-listener", func(wc *mgr.WorkerCtx, s struct{}) (cancel bool, err error) {
err = initializeSessions(module, listener)
if err != nil {
+ log.Errorf("dnsmonitor: failed to initialize sessions after DLL load: %v", err)
return false, err
}
return true, nil
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| err := initializeSessions(module, listener) | |
| if err != nil { | |
| return nil, err | |
| // Listen for event if the dll has been loaded | |
| module.instance.OSIntegration().OnInitializedEvent.AddCallback("loader-listener", func(wc *mgr.WorkerCtx, s struct{}) (cancel bool, err error) { | |
| err = initializeSessions(module, listener) | |
| if err != nil { | |
| return false, err | |
| } | |
| return true, nil | |
| }) | |
| } | |
| err := initializeSessions(module, listener) | |
| if err != nil { | |
| // Listen for event if the dll has been loaded | |
| module.instance.OSIntegration().OnInitializedEvent.AddCallback("loader-listener", func(wc *mgr.WorkerCtx, s struct{}) (cancel bool, err error) { | |
| err = initializeSessions(module, listener) | |
| if err != nil { | |
| log.Errorf("dnsmonitor: failed to initialize sessions after DLL load: %v", err) | |
| return false, err | |
| } | |
| return true, nil | |
| }) | |
| } |
| func initializeSessions(module *DNSMonitor, listener *Listener) error { | ||
| var err error | ||
| listener.etw, err = NewSession(module.instance.OSIntegration().GetETWInterface(), listener.processEvent) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Start listener | ||
| module.mgr.Go("etw-dns-event-listener", func(w *mgr.WorkerCtx) error { | ||
| return listener.etw.StartTrace() | ||
| }) | ||
|
|
||
| return listener, nil | ||
| return nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider cleanup on initialization failure
If StartTrace fails in the worker, the ETW session remains initialized. Consider adding cleanup.
func initializeSessions(module *DNSMonitor, listener *Listener) error {
var err error
listener.etw, err = NewSession(module.instance.OSIntegration().GetETWInterface(), listener.processEvent)
if err != nil {
return err
}
+
+ initialized := make(chan error, 1)
// Start listener
module.mgr.Go("etw-dns-event-listener", func(w *mgr.WorkerCtx) error {
- return listener.etw.StartTrace()
+ if err := listener.etw.StartTrace(); err != nil {
+ initialized <- err
+ return err
+ }
+ initialized <- nil
+ return nil
})
+
+ // Wait for initialization
+ if err := <-initialized; err != nil {
+ _ = listener.etw.DestroySession()
+ listener.etw = nil
+ return fmt.Errorf("failed to start trace: %w", err)
+ }
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func initializeSessions(module *DNSMonitor, listener *Listener) error { | |
| var err error | |
| listener.etw, err = NewSession(module.instance.OSIntegration().GetETWInterface(), listener.processEvent) | |
| if err != nil { | |
| return err | |
| } | |
| // Start listener | |
| module.mgr.Go("etw-dns-event-listener", func(w *mgr.WorkerCtx) error { | |
| return listener.etw.StartTrace() | |
| }) | |
| return listener, nil | |
| return nil | |
| func initializeSessions(module *DNSMonitor, listener *Listener) error { | |
| var err error | |
| listener.etw, err = NewSession(module.instance.OSIntegration().GetETWInterface(), listener.processEvent) | |
| if err != nil { | |
| return err | |
| } | |
| initialized := make(chan error, 1) | |
| // Start listener | |
| module.mgr.Go("etw-dns-event-listener", func(w *mgr.WorkerCtx) error { | |
| if err := listener.etw.StartTrace(); err != nil { | |
| initialized <- err | |
| return err | |
| } | |
| initialized <- nil | |
| return nil | |
| }) | |
| // Wait for initialization | |
| if err := <-initialized; err != nil { | |
| _ = listener.etw.DestroySession() | |
| listener.etw = nil | |
| return fmt.Errorf("failed to start trace: %w", err) | |
| } | |
| return nil | |
| } |
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor