Feature/systemd query events #1728
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request encompass updates to various files across multiple components of the project. Key modifications include the addition of new dependencies in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DNSMonitor
participant ETWSession
participant ResolverModule
User->>DNSMonitor: Start monitoring DNS events
DNSMonitor->>ETWSession: Create new session
ETWSession->>DNSMonitor: Session created
DNSMonitor->>ResolverModule: Check resolver state
ResolverModule-->>DNSMonitor: Resolver enabled/disabled
DNSMonitor->>User: Monitoring started
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: 7
🧹 Outside diff range and nitpick comments (15)
service/firewall/bypassing.go (2)
49-49: Consider strengthening the system resolver check.The TODO comment suggests uncertainty about the specificity of the check. Consider adding additional validation such as verifying the process path or using a more comprehensive identification method.
46-56: Document the resolver disabled behavior.Given that this introduces significant new behavior when the resolver is disabled, consider adding a function-level comment explaining:
- When and why the resolver might be disabled
- Security implications of the disabled state
- Interaction with systemd-resolver
service/firewall/interception/dnslistener/varlinktypes.go (3)
1-4: Enhance package documentation.Consider adding more detailed documentation about:
- The varlink protocol and its role in systemd-resolver
- The relationship between these types and the DNS event handling
- Any relevant systemd-resolver documentation references
package dnslistener -// List of struct that define the systemd-resolver varlink dns event protocol. +// Package dnslistener provides types for interacting with systemd-resolver's varlink DNS event protocol. +// These types facilitate the communication between the application and systemd-resolver for DNS query events. +// +// For more information about the protocol, see: +// https://www.freedesktop.org/software/systemd/man/org.freedesktop.resolve1.html
5-9: Add field documentation and consider using DNS-specific types.The struct would benefit from:
- Documentation explaining the purpose of each field
- Using DNS-specific constants for Class and Type fields
+// ResourceKey represents a unique identifier for a DNS resource record. type ResourceKey struct { + // Class represents the DNS class (e.g., IN for Internet) Class int `json:"class"` + // Type represents the DNS record type (e.g., A, AAAA, MX) Type int `json:"type"` + // Name is the domain name for this resource Name string `json:"name"` }Consider defining constants for common DNS classes and types:
// DNS Classes const ( ClassIN = 1 // Internet ClassCS = 2 // CSNET ClassCH = 3 // CHAOS ClassHS = 4 // Hesiod ) // DNS Types const ( TypeA = 1 // IPv4 address TypeNS = 2 // Nameserver TypeCNAME = 5 // Canonical name TypeSOA = 6 // Start of authority TypePTR = 12 // Pointer TypeMX = 15 // Mail exchange TypeTXT = 16 // Text TypeAAAA = 28 // IPv6 address )
65-69: Add struct and field documentation.The struct would benefit from clear documentation explaining its purpose and the significance of each field.
+// Answer represents a DNS response containing either a parsed resource record +// or the raw DNS response data. type Answer struct { + // RR is the parsed DNS resource record, if available RR *ResourceRecord `json:"rr,omitempty"` + // Raw contains the unparsed DNS response string Raw string `json:"raw"` + // IfIndex is the network interface index where this answer was received IfIndex *int `json:"ifindex,omitempty"` }service/compat/module.go (2)
102-106: Consider standardizing skip handling witherrSelfcheckSkipped.The early return for disabled resolver is logically sound, but for consistency with other skip conditions (like the one handled by
errSelfcheckSkipped), consider using the same error pattern.res := module.instance.Resolver() if res.IsDisabled.IsSet() { - log.Debugf("compat: skipping self-check: resolver is disabled") - return nil + return nil, fmt.Errorf("%w: resolver is disabled", errSelfcheckSkipped) }
192-192: LGTM! Consider documenting interface changes.The new
Resolver()method is a good addition for accessing resolver state. Consider adding interface documentation to explain the contract, especially regarding initialization guarantees and thread safety requirements.go.mod (1)
Line range hint
5-6: Track temporary winres dependency replacement.The TODO comment indicates this is a temporary fix waiting for upstream PR #4. Consider:
- Adding a GitHub issue to track the removal of this replacement
- Adding a link to the upstream PR for better context
Would you like me to create a GitHub issue to track this?
service/resolver/main.go (2)
32-33: Add documentation for the exported field.Since
IsDisabledis an exported field that other packages may use, please add a documentation comment explaining its purpose, when it's set, and how it should be used.+// IsDisabled indicates whether the resolver module is currently disabled. +// This is typically set when no resolvers are configured. IsDisabled abool.AtomicBool
32-33: Consider reflecting the disabled state in the state manager.The module uses
mgr.StateMgrfor state management, but the newIsDisabledstate is not reflected there. Consider adding the disabled state to the state manager to maintain consistency and enable proper monitoring.This could be done in the
loadResolversfunction (whereIsDisabledis set) by adding:if module.IsDisabled.IsSet() { module.states.Add("resolver:disabled", "The resolver module is currently disabled as no resolvers are configured") } else { module.states.Remove("resolver:disabled") }Also applies to: 272-274
service/instance.go (2)
192-195: Fix error message for consistencyThe error message uses "dns-listener" with a hyphen, while other module names in error messages don't use hyphens. Consider changing for consistency:
- return instance, fmt.Errorf("create dns-listener module: %w", err) + return instance, fmt.Errorf("create dnslistener module: %w", err)
473-476: Fix comment style for consistencyThe comment uses "dns-listener" with a hyphen, while other module names in comments don't use hyphens. Consider changing for consistency:
-// DNSListener returns the dns-listener module. +// DNSListener returns the dnslistener module.service/firewall/packet_handler.go (1)
Line range hint
447-459: Security review of DNS redirection logic.The implementation maintains robust security controls while adding flexibility:
- Multiple validation layers (resolver status, nameserver IP, port checks)
- Special handling for broadcast queries
- Proper separation between system and user DNS queries
Consider adding debug logging when DNS redirection is skipped due to disabled resolver to aid in troubleshooting.
Add debug logging:
case dnsQueryInterception() && module.instance.Resolver().IsDisabled.IsNotSet() && pkt.IsOutbound() && + // Log when redirection is skipped due to disabled resolver + func() bool { + if module.instance.Resolver().IsDisabled.IsSet() { + log.Tracer(pkt.Ctx()).Debugf("filter: skipping DNS redirection, resolver is disabled: %s", pkt) + } + return true + }() && pkt.Info().DstPort == 53 &&service/firewall/interception/dnslistener/module.go (2)
9-14: Organize imports according togcirecommendationsThe imports are not properly organized as indicated by the
gcistatic analysis tool. Consider runninggciwith--skip-generated -s standard -s defaultto sort and group imports according to Go standards.🧰 Tools
🪛 GitHub Check: Linter
[failure] 9-9:
File is notgci-ed with --skip-generated -s standard -s default (gci)
[failure] 14-14:
File is notgci-ed with --skip-generated -s standard -s default (gci)
144-146: Handle error frominfo.Save()appropriatelyCurrently, when
info.Save()returns an error, it is logged but not returned or handled further. Consider whether this error should be propagated or if additional error handling is necessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (10)
go.mod(2 hunks)service/compat/module.go(3 hunks)service/firewall/bypassing.go(1 hunks)service/firewall/interception/dnslistener/module.go(1 hunks)service/firewall/interception/dnslistener/varlinktypes.go(1 hunks)service/firewall/module.go(3 hunks)service/firewall/packet_handler.go(1 hunks)service/instance.go(5 hunks)service/resolver/main.go(2 hunks)service/resolver/resolvers.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Linter
service/firewall/interception/dnslistener/module.go
[failure] 9-9:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 14-14:
File is not gci-ed with --skip-generated -s standard -s default (gci)
[failure] 48-48:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 51-51:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
[failure] 60-60:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 63-63:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
🔇 Additional comments (13)
service/firewall/bypassing.go (2)
46-56: Verify interaction with secure DNS bypass reporting.
The new exceptions for disabled resolver state are placed before the secure DNS filter list checks. This means connections that would normally trigger secure DNS bypass reporting might be allowed when the resolver is disabled.
Let's check the interaction:
#!/bin/bash
# Description: Verify how secure DNS bypass reporting works with disabled resolver
# Test: Check for any conditions or checks around secure DNS bypass reporting
rg "ReportSecureDNSBypassIssue"46-56: Verify security implications of bypass exceptions.
The new conditional block creates exceptions to bypass prevention when the Portmaster resolver is disabled. While the intent is clear, we should ensure these exceptions don't create security vulnerabilities.
Let's verify the impact and usage:
✅ Verification successful
The bypass exceptions are properly implemented with adequate security measures
The verification reveals that the bypass exceptions are well-integrated into the system's security architecture:
-
The system has comprehensive DNS bypass prevention:
- Bypass attempts trigger security notifications via
ReportSecureDNSBypassIssue - There's a global setting
Block Secure DNS Bypassingthat's enabled by default - The firewall maintains protection even when bypassing is attempted
- Bypass attempts trigger security notifications via
-
The localhost and system resolver exceptions are specifically handled:
- System resolver checks are consistently implemented across the codebase
- Localhost scope checks are used in multiple security contexts with proper validation
- The exceptions only apply when the Portmaster resolver is explicitly disabled
-
The conditional block maintains security by:
- Only allowing necessary traffic when the resolver is disabled
- Properly identifying system resolver processes
- Limiting localhost exceptions to DNS port 53
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other security measures or validations when resolver is disabled
# Test 1: Check for any additional security checks or validations when resolver is disabled
rg -A 5 "IsDisabled.IsSet\(\)"
# Test 2: Check how localhost connections are handled elsewhere
rg -A 5 "IsLocalhost\(\)"
Length of output: 400
Script:
#!/bin/bash
# Let's try a more comprehensive verification approach
# Check for IsDisabled usage with proper quoting
rg -A 5 "IsDisabled\.IsSet"
# Check how localhost connections are validated
rg -A 5 "IsLocalhost"
# Check for system resolver related checks
rg -A 5 "IsSystemResolver"
# Check for other DNS bypass prevention mechanisms
rg -A 5 "DNS.*bypass" -i
# Look for related security validations
rg -A 5 "ReportSecureDNSBypassIssue"
Length of output: 13343
service/firewall/interception/dnslistener/varlinktypes.go (1)
71-79: Add documentation and validate pointer usage.
The struct needs documentation, and some fields might not need to be pointers.
+// QueryResult represents the complete result of a DNS query from systemd-resolver,
+// including the query state, any errors, and the received answers.
type QueryResult struct {
+ // Ready indicates if the query has completed
Ready *bool `json:"ready,omitempty"`
+ // State describes the current state of the query
State *string `json:"state,omitempty"`
+ // Rcode is the DNS response code
Rcode *int `json:"rcode,omitempty"`
+ // Errno is the system error number, if any
Errno *int `json:"errno,omitempty"`
+ // Question contains the original DNS query
Question *[]ResourceKey `json:"question,omitempty"`
+ // CollectedQuestions contains additional related queries
CollectedQuestions *[]ResourceKey `json:"collectedQuestions,omitempty"`
+ // Answer contains the DNS responses
Answer *[]Answer `json:"answer,omitempty"`
}Consider whether some fields really need to be pointers. For example:
Readycould be a non-pointerboolwith a default value offalseStatecould be an enum type instead of a string pointerRcodeandErrnocould be non-pointer integers with sentinel values (-1) for "not set"
service/firewall/module.go (1)
19-19: LGTM: Clean integration of resolver module access.
The addition of the resolver module access follows the established pattern of module getter methods in the interface. This change maintains consistency with the existing architecture.
Let's verify that all implementations of the instance interface have been updated:
Also applies to: 168-168
✅ Verification successful
Let me gather more information to verify the interface implementation comprehensively.
LGTM: The resolver module integration is properly implemented.
The verification confirms that:
- The
Instancestruct is the only implementation of theinstanceinterface - The
Instancestruct has both the required fieldresolver *resolver.ResolverModuleand the methodResolver() *resolver.ResolverModule - All interface methods are implemented by the
Instancestruct
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all potential implementations of the instance interface
# to ensure they've been updated with the new Resolver() method.
# Search for struct types that might implement the interface
ast-grep --pattern 'type $name struct {
$$$
}'
# Search for interface method implementations
rg -A 1 'func \([^)]+\) Resolver\(\)'
Length of output: 206
Script:
#!/bin/bash
# First, let's find the interface definition to understand all required methods
rg -A 10 "type instance interface" service/firewall/module.go
# Then find all structs that could implement this interface
rg "type.*struct" service/instance.go
# Finally, check if Instance is the only implementation
fd -e go | xargs rg "type.*struct.*{"
Length of output: 496
Script:
#!/bin/bash
# Let's fix the regex and search for struct implementations
fd -e go | xargs rg "type.*struct"
# Also check if Instance implements all interface methods
rg "func \(i \*Instance\)" service/instance.go
# And verify the resolver field exists in Instance
rg "resolver.*\*resolver\.ResolverModule" service/instance.go
Length of output: 40121
service/compat/module.go (1)
127-128:
Potential race condition in resolver state handling.
The early return (lines 102-106) should prevent execution from reaching this point when the resolver is disabled. The fact that we're checking again suggests the resolver state could change during self-check execution. Consider:
- Adding atomic operations or proper synchronization
- Removing this redundant check
- Adding a comment explaining why both checks are necessary
Let's verify if the resolver state can change during execution:
go.mod (2)
37-37: Document new dependency purposes.
Two new direct dependencies have been added:
github.com/maruel/panicparse/v2: Consider documenting why this debugging tool is neededgithub.com/varlink/go: Required for systemd-resolver DNS event protocol integration
#!/bin/bash
# Check for usage of new dependencies in codebase
echo "Checking panicparse usage..."
rg "panicparse" --type go
echo "Checking varlink usage..."
rg "varlink" --type goConsider adding comments in the code or documentation explaining the purpose of these dependencies.
Also applies to: 61-61
Line range hint 3-3: Verify Go 1.22.0 compatibility requirements.
Go 1.22.0 is a very recent version (released Feb 2024). Please ensure all deployment environments and CI/CD pipelines support this version.
service/instance.go (4)
22-22: LGTM!
The import is correctly placed and follows the project's import grouping convention.
78-78: LGTM!
The dnslistener field is correctly placed and follows the established pattern for module fields.
297-297: LGTM!
The dnslistener module is correctly registered in the service group, maintaining the logical grouping with other interception-related modules.
78-78: Verify SleepyModule interface implementation
Since this is a new module in the service group, verify if it needs to implement the SleepyModule interface like other modules. This interface is used by the SetSleep method to manage module sleep states.
Also applies to: 192-195, 297-297, 473-476
service/resolver/resolvers.go (1)
401-408: 🛠️ Refactor suggestion
Consider moving the disabled state management after resolver validation.
The current implementation sets the module's disabled state before validating resolvers and potentially loading defaults. This could lead to incorrect state representation if:
- The configured resolvers are invalid and defaults are loaded
- Other modules check the disabled state while resolvers are being loaded
Consider moving this block after the resolver validation and default loading (after line 440) to ensure the state accurately reflects the final resolver configuration:
- // If no resolvers are configure set the disabled state. So other modules knows that the users does not want to use Portmaster resolver.
- if len(newResolverConfig) == 0 {
- module.IsDisabled.Set()
- } else {
- module.IsDisabled.UnSet()
- }
currentResolverConfig = newResolverConfig
newResolvers := append(
getConfiguredResolvers(newResolverConfig),
getSystemResolvers()...,
)
if len(newResolvers) == 0 {
// load defaults directly, overriding config system
newResolvers = getConfiguredResolvers(defaultNameServers)
...
}
+
+ // Set the disabled state after resolver validation and default loading
+ if len(newResolvers) == 0 {
+ module.IsDisabled.Set()
+ } else {
+ module.IsDisabled.UnSet()
+ }Let's verify the impact of this state management on other modules:
service/firewall/packet_handler.go (1)
447-449: LGTM! Verify DNS redirection behavior.
The additional check for resolver status is correctly implemented, ensuring DNS queries are only redirected when the resolver is enabled.
Let's verify the resolver status handling:
✅ Verification successful
Let me gather more information about the DNS redirection logic to ensure a comprehensive verification.
DNS redirection logic is correctly implemented with resolver status check
The verification confirms that:
- The resolver status check
IsDisabled.IsNotSet()is consistently used across the codebase for DNS handling - The DNS redirection logic is properly integrated with the resolver status:
- In packet handling (
service/firewall/packet_handler.go) - In bypassing logic (
service/firewall/bypassing.go) - In compatibility checks (
service/compat/module.go)
- In packet handling (
- The implementation correctly ensures DNS queries are only redirected when:
- The resolver is enabled (IsDisabled.IsNotSet())
- The packet is outbound
- The destination port is 53
- The destination is not our nameserver
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent resolver status handling across the codebase
# Test: Search for other DNS redirection logic that might need similar checks
rg -A 5 "DstPort == 53"
# Test: Search for other resolver status checks to ensure consistent patterns
ast-grep --pattern 'Resolver().IsDisabled'
Length of output: 895
Script:
#!/bin/bash
# Search for DNS query interception function and its usage
ast-grep --pattern 'dnsQueryInterception()'
# Search for resolver status checks in a broader context
rg -A 5 "IsDisabled"
# Look for nameserver related functions to understand DNS handling
rg -A 5 "RerouteToNameserver"
Length of output: 11331
| type ResourceRecord struct { | ||
| Key ResourceKey `json:"key"` | ||
| Name *string `json:"name,omitempty"` | ||
| Address *[]byte `json:"address,omitempty"` | ||
| // Rest of the fields are not used. | ||
| // Priority *int `json:"priority,omitempty"` | ||
| // Weight *int `json:"weight,omitempty"` | ||
| // Port *int `json:"port,omitempty"` | ||
| // CPU *string `json:"cpu,omitempty"` | ||
| // OS *string `json:"os,omitempty"` | ||
| // Items *[]string `json:"items,omitempty"` | ||
| // MName *string `json:"mname,omitempty"` | ||
| // RName *string `json:"rname,omitempty"` | ||
| // Serial *int `json:"serial,omitempty"` | ||
| // Refresh *int `json:"refresh,omitempty"` | ||
| // Expire *int `json:"expire,omitempty"` | ||
| // Minimum *int `json:"minimum,omitempty"` | ||
| // Exchange *string `json:"exchange,omitempty"` | ||
| // Version *int `json:"version,omitempty"` | ||
| // Size *int `json:"size,omitempty"` | ||
| // HorizPre *int `json:"horiz_pre,omitempty"` | ||
| // VertPre *int `json:"vert_pre,omitempty"` | ||
| // Latitude *int `json:"latitude,omitempty"` | ||
| // Longitude *int `json:"longitude,omitempty"` | ||
| // Altitude *int `json:"altitude,omitempty"` | ||
| // KeyTag *int `json:"key_tag,omitempty"` | ||
| // Algorithm *int `json:"algorithm,omitempty"` | ||
| // DigestType *int `json:"digest_type,omitempty"` | ||
| // Digest *string `json:"digest,omitempty"` | ||
| // FPType *int `json:"fptype,omitempty"` | ||
| // Fingerprint *string `json:"fingerprint,omitempty"` | ||
| // Flags *int `json:"flags,omitempty"` | ||
| // Protocol *int `json:"protocol,omitempty"` | ||
| // DNSKey *string `json:"dnskey,omitempty"` | ||
| // Signer *string `json:"signer,omitempty"` | ||
| // TypeCovered *int `json:"type_covered,omitempty"` | ||
| // Labels *int `json:"labels,omitempty"` | ||
| // OriginalTTL *int `json:"original_ttl,omitempty"` | ||
| // Expiration *int `json:"expiration,omitempty"` | ||
| // Inception *int `json:"inception,omitempty"` | ||
| // Signature *string `json:"signature,omitempty"` | ||
| // NextDomain *string `json:"next_domain,omitempty"` | ||
| // Types *[]int `json:"types,omitempty"` | ||
| // Iterations *int `json:"iterations,omitempty"` | ||
| // Salt *string `json:"salt,omitempty"` | ||
| // Hash *string `json:"hash,omitempty"` | ||
| // CertUsage *int `json:"cert_usage,omitempty"` | ||
| // Selector *int `json:"selector,omitempty"` | ||
| // MatchingType *int `json:"matching_type,omitempty"` | ||
| // Data *string `json:"data,omitempty"` | ||
| // Tag *string `json:"tag,omitempty"` | ||
| // Value *string `json:"value,omitempty"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve documentation and consider separating unused fields.
The current approach of commenting out unused fields makes the code harder to maintain and understand.
Consider either:
- Moving unused fields to a separate file/type for future implementation
- Or documenting why these fields exist and when they might be needed
+// ResourceRecord represents a DNS resource record with its associated data.
+// Currently, only a subset of DNS record types are supported, focusing on
+// name resolution and addressing. Additional fields for other DNS record
+// types are planned for future implementation.
type ResourceRecord struct {
Key ResourceKey `json:"key"`
+ // Name is the canonical name for CNAME records
Name *string `json:"name,omitempty"`
+ // Address contains the IP address for A/AAAA records
Address *[]byte `json:"address,omitempty"`
- // Rest of the fields are not used.
- // Priority *int `json:"priority,omitempty"`
- // Weight *int `json:"weight,omitempty"`
- // Port *int `json:"port,omitempty"`
- // ... (other commented fields)
}Create a new file (e.g., future_types.go) for the unused fields:
// future_types.go
// FutureResourceRecord contains fields for additional DNS record types
// that may be implemented in the future.
type FutureResourceRecord struct {
Priority *int `json:"priority,omitempty"`
Weight *int `json:"weight,omitempty"`
Port *int `json:"port,omitempty"`
// ... (other fields)
}
service/resolver/main.go
Outdated
| mgr: m, | ||
| instance: instance, | ||
| IsDisabled: *abool.New(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unnecessary dereferencing of abool.New().
The abool.New() function returns a pointer to AtomicBool, but the field type is not a pointer. The dereferencing is unnecessary and could be error-prone.
- IsDisabled: *abool.New(),
+ IsDisabled: abool.New(),📝 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.
| mgr: m, | |
| instance: instance, | |
| IsDisabled: *abool.New(), | |
| mgr: m, | |
| instance: instance, | |
| IsDisabled: abool.New(), |
| if err != nil { | ||
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | ||
| return nil |
There was a problem hiding this comment.
Return error instead of nil on connection failure
In the Start() method, when failing to establish a varlink connection, the error is logged but nil is returned. Consider returning the error to allow the caller to handle it appropriately.
Apply this diff to fix the issue:
if err != nil {
log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err)
- return nil
+ return err
}📝 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.
| if err != nil { | |
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | |
| return nil | |
| if err != nil { | |
| log.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %s", err) | |
| return err |
| if varlinkErr, ok := err.(*varlink.Error); ok { | ||
| return fmt.Errorf("failed to issue Varlink call: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to issue Varlink call: %v", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap errors using %w in fmt.Errorf
When returning errors with fmt.Errorf, use the %w verb to wrap the original error and maintain the error chain.
Apply this diff to refactor:
-return fmt.Errorf("failed to issue Varlink call: %v", err)
+return fmt.Errorf("failed to issue Varlink call: %w", err)📝 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.
| return fmt.Errorf("failed to issue Varlink call: %v", err) | |
| return fmt.Errorf("failed to issue Varlink call: %w", err) |
🧰 Tools
🪛 GitHub Check: Linter
[failure] 51-51:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
| if varlinkErr, ok := err.(*varlink.Error); ok { | ||
| return fmt.Errorf("failed to receive Varlink reply: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to receive Varlink reply: %v", err) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap errors using %w in fmt.Errorf
Again, use %w to wrap errors when returning them with fmt.Errorf.
Apply this diff to refactor:
-return fmt.Errorf("failed to receive Varlink reply: %v", err)
+return fmt.Errorf("failed to receive Varlink reply: %w", err)📝 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.
| return fmt.Errorf("failed to receive Varlink reply: %v", err) | |
| return fmt.Errorf("failed to receive Varlink reply: %w", err) |
🧰 Tools
🪛 GitHub Check: Linter
[failure] 63-63:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
| if varlinkErr, ok := err.(*varlink.Error); ok { | ||
| return fmt.Errorf("failed to receive Varlink reply: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to receive Varlink reply: %v", err) |
There was a problem hiding this comment.
Use errors.As for error type assertions
Similar to the previous case, use errors.As to handle wrapped errors when checking for specific error types.
Apply this diff to fix the issue:
-if varlinkErr, ok := err.(*varlink.Error); ok {
+var varlinkErr *varlink.Error
+if errors.As(err, &varlinkErr) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Linter
[failure] 60-60:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 63-63:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
| if varlinkErr, ok := err.(*varlink.Error); ok { | ||
| return fmt.Errorf("failed to issue Varlink call: %+v", varlinkErr.Parameters) | ||
| } else { | ||
| return fmt.Errorf("failed to issue Varlink call: %v", err) |
There was a problem hiding this comment.
Use errors.As for error type assertions
Type assertions on errors may fail on wrapped errors. Use errors.As to check for specific error types, ensuring wrapped errors are properly handled.
Apply this diff to fix the issue:
-if varlinkErr, ok := err.(*varlink.Error); ok {
+var varlinkErr *varlink.Error
+if errors.As(err, &varlinkErr) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: Linter
[failure] 48-48:
type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint)
[failure] 51-51:
non-wrapping format verb for fmt.Errorf. Use %w to format errors (errorlint)
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (10)
service/network/connection.go (1)
552-552: Consider handling the Flush error.The error from
DNSListener().Flush()is being silently ignored. While this might be intentional, it's worth logging the error for debugging purposes.- _ = module.instance.DNSListener().Flush() + if err := module.instance.DNSListener().Flush(); err != nil { + log.Tracer(pkt.Ctx()).Debugf("network: failed to flush dns listener: %v", err) + }service/firewall/interception/dnslistener/eventlistener_linux.go (4)
86-86: Correct typos in comments for better readabilityThere are typographical errors in the comments that should be corrected:
- Line 86: "Allocated data struct for the parsed result." should be "Allocate data structures for the parsed result."
- Line 97: "Go trough each answer entry." should be "Go through each answer entry."
Making these corrections enhances the readability and professionalism of the code.
Also applies to: 97-97
143-145: Consider additional error handling when saving IP info recordsWhen
info.Save()fails, the error is logged but not otherwise handled. Depending on the importance of this operation, additional actions might be necessary.Consider implementing retry logic or propagating the error to ensure the issue is addressed appropriately.
if err := info.Save(); err != nil { log.Errorf("nameserver: failed to save IP info record: %s", err) + // Additional error handling here }
78-83: SetvarlinkConntonilafter closingAfter closing the
varlinkConn, it's a good practice to set it tonilto prevent potential nil-pointer dereferences or misuse after closure.Apply this diff to update the
stopmethod:func (l *Listener) stop() error { if l.varlinkConn != nil { err := l.varlinkConn.Close() + l.varlinkConn = nil return err } return nil }
25-27: Improve error handling when Varlink service is unavailableIf the Varlink connection to the systemd resolver cannot be established, the function returns an error immediately.
Consider implementing a retry mechanism or handling the error gracefully to improve resilience against temporary service unavailability.
if err != nil { - return nil, fmt.Errorf("dnslistener: failed to connect to systemd-resolver varlink service: %w", err) + log.Warnf("dnslistener: failed to connect, retrying: %s", err) + // Implement retry logic here }service/firewall/interception/dnslistener/etwlink_windows.go (5)
28-28: Correct typos in the function comment.In the comment for
NewSession, "initilizes" should be "initializes", and "DestorySession" should be "DestroySession". This improves readability and professionalism.Apply this diff to fix the typos:
-// NewSession creates new ETW event listener and initilizes it. This is a low level interface, make sure to call DestorySession when you are done using it. +// NewSession creates a new ETW event listener and initializes it. This is a low-level interface; make sure to call DestroySession when you are done using it.
36-36: Fix typos in error messages when loading DLL functions.There are several instances where "faild" should be "failed" in the error messages. Correcting these typos enhances clarity and professionalism.
Apply this diff to correct the typos:
- return nil, fmt.Errorf("faild to load dll: %q", err) + return nil, fmt.Errorf("failed to load dll: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWCreateState: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWCreateState: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWInitializeSession: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWInitializeSession: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWStartTrace: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWStartTrace: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWFlushTrace: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWFlushTrace: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWStopTrace: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWStopTrace: %q", err) - return nil, fmt.Errorf("faild to load function PM_ETWDestroySession: %q", err) + return nil, fmt.Errorf("failed to load function PM_ETWDestroySession: %q", err)Also applies to: 40-40, 44-44, 48-48, 52-52, 56-56, 60-60
80-80: Correct the typo in the error message."initialzie" should be "initialize" in the error message for clarity.
Apply this diff to fix the typo:
- return nil, fmt.Errorf("failed to initialzie session: error code: %q", rc) + return nil, fmt.Errorf("failed to initialize session: error code: %q", rc)
112-112: Fix the typo in the function comment.In the comment for
StopTrace, "stopes" should be "stops" to correct the spelling.Apply this diff to correct the typo:
-// StopTrace stopes the trace. This will cause StartTrace to return. +// StopTrace stops the trace. This will cause StartTrace to return.
69-69: Handle potential errors fromcreateState.Call().Even if the function is expected not to fail, it's good practice to check and handle any unexpected errors from
createState.Call(). This can prevent potential issues if assumptions change in the future or under different environments.Consider modifying the code to capture and handle any errors:
- etwListener.state, _, _ = etwListener.createState.Call(win32Callback) + etwListener.state, _, err = etwListener.createState.Call(win32Callback) + if etwListener.state == 0 || err != nil { + return nil, fmt.Errorf("failed to create state: %v", err) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
service/firewall/interception/dnslistener/etwlink_windows.go(1 hunks)service/firewall/interception/dnslistener/eventlistener.go(1 hunks)service/firewall/interception/dnslistener/eventlistener_linux.go(1 hunks)service/firewall/interception/dnslistener/eventlistener_windows.go(1 hunks)service/firewall/interception/dnslistener/module.go(1 hunks)service/firewall/interception/dnslistener/varlinktypes.go(1 hunks)service/network/connection.go(2 hunks)service/network/module.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- service/firewall/interception/dnslistener/eventlistener.go
🚧 Files skipped from review as they are similar to previous changes (2)
- service/firewall/interception/dnslistener/module.go
- service/firewall/interception/dnslistener/varlinktypes.go
🔇 Additional comments (3)
service/network/module.go (1)
12-12: LGTM: Import additions are appropriate
The new imports for dnslistener and resolver packages align with the interface extensions and PR objectives.
Also applies to: 17-17
service/firewall/interception/dnslistener/eventlistener_linux.go (2)
125-134: Verify modification of domain within the loop
In the loop processing CNAME records, the variable domain is reassigned:
domain = nextDomainThis could affect the outer scope if domain is needed after the loop.
Please verify that changing domain within this loop is intentional and does not impact subsequent code that relies on the original domain value.
121-121:
Undefined variable ResolverInfo
The variable ResolverInfo used here is not defined within the current scope or imported from another package. This will result in a compilation error.
Please define ResolverInfo or import it from the appropriate package. If it's meant to be a reference to an existing resolver, ensure it's correctly initialized and accessible.
Apply this diff if ResolverInfo should reference an existing variable:
+var ResolverInfo = resolver.ResolverInfo{
+ // Initialize with appropriate values
+}Likely invalid or redundant comment.
service/network/module.go
Outdated
| Resolver() *resolver.ResolverModule | ||
| DNSListener() *dnslistener.DNSListener |
There was a problem hiding this comment.
💡 Codebase verification
Based on all the gathered information, I can now provide the final response:
Interface implementations need to be updated with new methods
The interface instance in service/network/module.go has been extended with two new methods:
Resolver() *resolver.ResolverModuleDNSListener() *dnslistener.DNSListener
However, the implementations of these methods are missing in the codebase. This needs to be addressed to maintain interface compatibility.
- The main implementation needs to be updated to include these methods
- Test implementations in
service/network/*_test.goneed to be updated to include these methods
🔗 Analysis chain
Verify implementation of new interface methods
The interface additions look good, but we should verify that all implementers of the instance interface have been updated with the new methods.
Let me gather more information about the test instances and their implementations.
Let me search for the actual implementation of the network module's instance interface.
Let me search for the concrete implementation of these methods in the codebase.
Let me check for the actual implementation of the interface in the core module.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all potential implementers of the instance interface
ast-grep --pattern 'type $name struct {
$$$
}
func ($receiver *$name) Profile() *profile.ProfileModule {
$$$
}'
# Also check for any direct interface usage
rg "var\s+\w+\s+instance\s*=" -A 2
Length of output: 1034
Script:
#!/bin/bash
# Check test instance implementations
for file in service/resolver/main_test.go service/netenv/init_test.go service/intel/geoip/init_test.go base/api/init_test.go base/config/init_test.go; do
echo "=== $file ==="
cat "$file" | grep -A 20 "type testInstance"
done
# Find the actual instance interface definition
rg "type instance interface" -A 10
# Look for real implementations (non-test)
ast-grep --pattern 'type $name struct {
$$$
}
func ($receiver *$name) Resolver() *resolver.ResolverModule {
$$$
}'
Length of output: 9434
Script:
#!/bin/bash
# Find the main implementation of the network module
rg "type (\w+) struct" service/network/ -A 5
# Find implementations of Profile() method to identify concrete types
ast-grep --pattern 'func ($receiver *$name) Profile() *profile.ProfileModule {
$$$
}'
# Check if these types implement the new methods
ast-grep --pattern 'func ($receiver *$name) DNSListener() *dnslistener.DNSListener {
$$$
}'
Length of output: 10342
Script:
#!/bin/bash
# Find the main implementation file
rg "func New\(" service/network/module.go -A 10
# Find all implementations of the new methods
rg "func.*Resolver\(\)" service/network/
rg "func.*DNSListener\(\)" service/network/
# Find where the interface is used
rg "var\s+_\s+instance\s+=\s+&" service/network/
Length of output: 552
Script:
#!/bin/bash
# Find the core implementation
rg "type Core struct" service/core/ -A 10
# Check if Core implements these methods
ast-grep --pattern 'func ($receiver *Core) Resolver() *resolver.ResolverModule {
$$$
}'
ast-grep --pattern 'func ($receiver *Core) DNSListener() *dnslistener.DNSListener {
$$$
}'
# Find where Core is used as instance
rg "New\(.*Core.*\)" service/network/
Length of output: 728
service/network/connection.go
Outdated
| if runtime.GOOS == "windows" { | ||
| // On windows domains may come with delay. | ||
| if err != nil && module.instance.Resolver().IsDisabled.IsSet() { | ||
| // Flush the dns listener buffer and try again. | ||
| for i := range 4 { | ||
| _ = module.instance.DNSListener().Flush() | ||
| ipinfo, err = resolver.GetIPInfo(resolver.IPInfoProfileScopeGlobal, pkt.Info().RemoteIP().String()) | ||
| if err == nil { | ||
| log.Tracer(pkt.Ctx()).Debugf("network: found domain from dnslistener after %d tries", i+1) | ||
| break | ||
| } | ||
| time.Sleep(5 * time.Millisecond) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider improving the retry mechanism configuration and error handling.
While the Windows-specific retry logic addresses DNS resolution delays, there are several areas for improvement:
- The magic numbers should be configurable constants
- The retry operation should have an overall timeout
- The sleep duration in the hot path could impact performance
Consider applying these improvements:
+const (
+ dnsRetryAttempts = 4
+ dnsRetrySleepMs = 5
+ dnsRetryTimeoutMs = 100
+)
if runtime.GOOS == "windows" {
// On windows domains may come with delay.
if err != nil && module.instance.Resolver().IsDisabled.IsSet() {
+ ctx, cancel := context.WithTimeout(pkt.Ctx(), time.Duration(dnsRetryTimeoutMs)*time.Millisecond)
+ defer cancel()
// Flush the dns listener buffer and try again.
- for i := range 4 {
+ for i := range dnsRetryAttempts {
+ select {
+ case <-ctx.Done():
+ log.Tracer(pkt.Ctx()).Debugf("network: dns retry timeout after %d attempts", i)
+ break
+ default:
_ = module.instance.DNSListener().Flush()
ipinfo, err = resolver.GetIPInfo(resolver.IPInfoProfileScopeGlobal, pkt.Info().RemoteIP().String())
if err == nil {
log.Tracer(pkt.Ctx()).Debugf("network: found domain from dnslistener after %d tries", i+1)
break
}
- time.Sleep(5 * time.Millisecond)
+ time.Sleep(time.Duration(dnsRetrySleepMs) * time.Millisecond)
+ }
}
}
}Committable suggestion skipped: line range outside the PR's diff.
| if err != nil { | ||
| return fmt.Errorf("StopTrace failed: %d", err) | ||
| } |
There was a problem hiding this comment.
Fix error messages: use correct format specifiers, variable names, and correct typos.
Several issues are present in the error handling:
- Format Specifiers: Use
%vinstead of%dto format error variables. - Variable Usage: Use the correct error variable (
err2) in the second error message. - Typo Correction: Correct "DestorySession" to "DestroySession".
Suggested changes:
if err != nil {
- return fmt.Errorf("StopTrace failed: %d", err)
+ return fmt.Errorf("StopTrace failed: %v", err)
}
if err2 != nil {
- return fmt.Errorf("DestorySession failed: %d", err)
+ return fmt.Errorf("DestroySession failed: %v", err2)
}Also applies to: 56-58
| // Create new record for this IP. | ||
| record := resolver.ResolvedDomain{ | ||
| Domain: fqdn, | ||
| Resolver: &ResolverInfo, |
There was a problem hiding this comment.
Ensure ResolverInfo is defined and accessible in the current scope.
The variable ResolverInfo is referenced but not defined within this file or imported from another package. This will lead to a compilation error due to an undefined identifier. Please define ResolverInfo or import it from the appropriate package.
| for { | ||
| nextDomain, isCNAME := cnames[domain] | ||
| if !isCNAME { | ||
| break | ||
| } | ||
|
|
||
| record.CNAMEs = append(record.CNAMEs, nextDomain) | ||
| domain = nextDomain | ||
| } |
There was a problem hiding this comment.
Prevent infinite loops by handling CNAME cycles in processEvent.
If there is a cyclic CNAME chain, the for loop can result in an infinite loop. To prevent this, keep track of visited domains and break the loop if a domain is revisited.
Suggested modification:
visited := make(map[string]bool)
for {
if visited[domain] {
// Detected a CNAME cycle
break
}
visited[domain] = true
nextDomain, isCNAME := cnames[domain]
if !isCNAME {
break
}
record.CNAMEs = append(record.CNAMEs, nextDomain)
domain = nextDomain
}| func newListener(m *mgr.Manager) (*Listener, error) { | ||
| listener := &Listener{} | ||
| var err error | ||
| listener.etw, err = NewSession("C:/Dev/ETWDNSTrace.dll", listener.processEvent) |
There was a problem hiding this comment.
Avoid hardcoding file paths; use configurable or relative paths.
Using a hardcoded absolute file path ("C:/Dev/ETWDNSTrace.dll") is not portable and will cause issues when running the application in different environments. Consider using a relative path or making the DLL path configurable to improve flexibility and maintainability.
Suggested change:
- listener.etw, err = NewSession("C:/Dev/ETWDNSTrace.dll", listener.processEvent)
+ listener.etw, err = NewSession(dllPath, listener.processEvent)Where dllPath is obtained from a configuration setting or computed based on the application's location.
Committable suggestion skipped: line range outside the PR's diff.
| } | ||
|
|
||
| for { | ||
| queryResult := QueryResult{} |
There was a problem hiding this comment.
Undefined type QueryResult
The type QueryResult is used but not defined or imported, leading to a compilation error.
Ensure that QueryResult is defined or imported from the correct package. If it's part of the Varlink interface, you might need to generate the Varlink interface code or import it properly.
For example, if QueryResult is defined in a Varlink-generated package:
+import "path/to/varlink/package"Also applies to: 85-85
| func (l *Listener) flish() error { | ||
| // Nothing to flush | ||
| return nil |
There was a problem hiding this comment.
Typo in method name flish; did you mean flush?
The method flish seems to be a misspelling of flush. Renaming it to flush will improve code clarity and maintain consistency with standard terminology.
Apply this diff to correct the method name:
-func (l *Listener) flish() error {
+func (l *Listener) flush() error {
// Nothing to flush
return nil
}Ensure that all references to flish are updated accordingly.
📝 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 (l *Listener) flish() error { | |
| // Nothing to flush | |
| return nil | |
| func (l *Listener) flush() error { | |
| // Nothing to flush | |
| return nil |
| win32Callback := windows.NewCallback(func(domain *uint16, result *uint16) uintptr { | ||
| callback(windows.UTF16PtrToString(domain), windows.UTF16PtrToString(result)) | ||
| return 0 | ||
| }) |
There was a problem hiding this comment.
Ensure the callback function is not garbage collected prematurely.
The win32Callback function passed to createState.Call() could be garbage collected if there are no references to it, potentially causing a crash when the callback is invoked. To prevent this, store a reference to the callback in the ETWSession struct.
Modify the ETWSession struct and code as follows to retain the callback:
type ETWSession struct {
dll *windows.DLL
+ callback uintptr // Keep a reference to prevent GC
// ...
}
// Initialize notification activated callback
win32Callback := windows.NewCallback(func(domain *uint16, result *uint16) uintptr {
callback(windows.UTF16PtrToString(domain), windows.UTF16PtrToString(result))
return 0
})
+ etwListener.callback = win32Callback
// The function only allocates memory; it will not fail.
etwListener.state, _, _ = etwListener.createState.Call(win32Callback)This ensures that win32Callback remains reachable and is not garbage collected while in use.
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Is this true? This seems like a good idea.
(I was referencing something from the bot, but this does not show up here.)
There was a problem hiding this comment.
@dhaavi Yes, this is true. In Go, when you pass a callback function to system calls like windows.NewCallback(), the garbage collector might collect it if there are no remaining references. This can lead to crashes when the callback is invoked by the system. By storing a reference to win32Callback in the ETWSession struct, you ensure that it remains accessible and prevent it from being garbage collected.
| // Make sure DestroySession is called even if caller forgets to call it. | ||
| runtime.SetFinalizer(etwListener, func(l *ETWSession) { | ||
| _ = l.DestroySession() | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid relying on finalizers for critical resource cleanup.
Using runtime.SetFinalizer to call DestroySession may not guarantee timely resource release because finalizers run at an indeterminate time after the object becomes unreachable. It's important to ensure that DestroySession is called explicitly when the session is no longer needed.
Consider removing the finalizer and enforcing that clients of ETWSession call DestroySession explicitly:
- // Make sure DestroySession is called even if caller forgets to call it.
- runtime.SetFinalizer(etwListener, func(l *ETWSession) {
- _ = l.DestroySession()
- })Alternatively, if you decide to keep the finalizer as a safety net, clearly document that callers must call DestroySession and should not rely on the finalizer for resource management.
📝 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.
| // Make sure DestroySession is called even if caller forgets to call it. | |
| runtime.SetFinalizer(etwListener, func(l *ETWSession) { | |
| _ = l.DestroySession() | |
| }) |
There was a problem hiding this comment.
Im calling DestroySession explicitly. This for resilience, if for some reason the DestorySession is not called because of panic or error. Do you think this is still a bad practice?
There was a problem hiding this comment.
@vlabo, thank you for the clarification. If you're explicitly calling DestroySession and using the finalizer as an additional safety net in case of panics or unexpected errors, that's understandable. While finalizers may not guarantee timely resource cleanup, using them as a backup can enhance resilience. Just make sure to document this behavior clearly so that future maintainers are aware of the intention.
(_/)❓
(•ᴗ•)
( )つ🔍
✏️ Learnings added
Learnt from: vlabo
PR: safing/portmaster#1728
File: service/firewall/interception/dnslistener/etwlink_windows.go:71-74
Timestamp: 2024-11-18T14:08:51.438Z
Learning: In the file `service/firewall/interception/dnslistener/etwlink_windows.go`, the developer is explicitly calling `DestroySession` and is using `runtime.SetFinalizer` as an additional safety net to enhance resilience in case `DestroySession` is not called due to a panic or error.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
vlabo
force-pushed
the
feature/systemd-query-events
branch
from
52d9eea
to
b3b1615
Compare
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (20)
service/integration/integration_windows.go (3)
1-2: Consider using only the new build constraint style.The file uses both old (
// +build windows) and new (//go:build windows) build constraint styles. While this works, it's recommended to standardize on the new style for better maintainability.-//go:build windows -// +build windows +//go:build windows
13-16: Add documentation for the OSSpecific struct.The struct and its fields would benefit from documentation comments explaining their purpose and usage, especially since they handle critical Windows-specific functionality.
+// OSSpecific holds Windows-specific resources and functionality type OSSpecific struct { + // dll holds the loaded portmaster-core.dll instance dll *windows.DLL + // etwFunctions contains the ETW (Event Tracing for Windows) function references etwFunctions ETWFunctions }
41-44: Add documentation about thread safety and usage.The method needs documentation about its thread safety guarantees and expected usage patterns, especially since it's dealing with Windows ETW functionality.
+// GetETWInterface returns the initialized ETW functions. +// This method is thread-safe and can be called from multiple goroutines. +// The returned ETWFunctions should not be stored beyond the lifetime of the OSIntegration instance. func (i *OSIntegration) GetETWInterface() ETWFunctions { return i.os.etwFunctions }service/integration/module.go (2)
11-19: Add documentation for struct fields.While the struct itself is documented, adding comments for each field would improve code maintainability:
m: Purpose of the managerstates: Role of the state manageros: Expected implementation of OSSpecific interfaceinstance: Usage of the instance interface// OSIntegration module provides special integration with the OS. type OSIntegration struct { + // m is the module manager handling the lifecycle m *mgr.Manager + // states manages the module's state transitions states *mgr.StateMgr + // os implements OS-specific integration functionality os OSSpecific + // instance provides access to updates functionality instance instance }
55-57: Document the instance interface.Add documentation explaining the purpose of this interface and its relationship with the updates package.
+// instance provides access to the updates functionality required by the OSIntegration module. type instance interface { Updates() *updates.Updates }service/firewall/interception/dnslistener/eventlistener_windows.go (2)
20-35: Consider enhancing error handling in the goroutine.While the initial error handling is good, the goroutine's error return is not being monitored. Consider adding error handling for
StartTrace()failures in the goroutine.module.mgr.Go("etw-dns-event-listener", func(w *mgr.WorkerCtx) error { - return listener.etw.StartTrace() + if err := listener.etw.StartTrace(); err != nil { + // Log the error or notify through a channel + return fmt.Errorf("failed to start ETW trace: %v", err) + } + return nil })
37-39: Add nil checks for defensive programming.Consider adding nil checks similar to the
stopmethod to prevent potential panic.func (l *Listener) flush() error { + if l == nil { + return fmt.Errorf("listener is nil") + } + if l.etw == nil { + return fmt.Errorf("invalid etw session") + } return l.etw.FlushTrace() }service/firewall/interception/dnslistener/etwlink_windows.go (2)
47-47: Fix typo in error message.There's a typo in "initialzie" which should be "initialize".
- return nil, fmt.Errorf("failed to initialzie session: %q", err) + return nil, fmt.Errorf("failed to initialize session: %q", err)
53-56: Document possible error conditions.The
StartTraceandStopTracemethods should document the possible error conditions that can be returned. This helps users of the API handle errors appropriately.Example documentation:
// StartTrace starts the tracing session of dns events. This is a blocking call. // It will not return until the trace is stopped. // Returns error if the trace session fails to start or if the session is already destroyed. func (l *ETWSession) StartTrace() error {Also applies to: 76-79
service/firewall/interception/dnslistener/module.go (3)
16-20: Add documentation for ResolverInfoConsider adding a doc comment explaining the purpose and usage of this global variable, as it seems to be an important configuration for the systemd resolver integration.
+// ResolverInfo defines the identity and type of the systemd resolver integration. var ResolverInfo = resolver.ResolverInfo{
22-27: Add documentation for DNSListener structThe struct appears to be a core component. Consider adding documentation explaining its purpose and responsibilities.
+// DNSListener manages DNS event listening and processing for systemd resolver integration. type DNSListener struct {
102-119: Add documentation for singleton implementationConsider adding documentation to explain:
- The purpose of shimLoaded and its thread-safety guarantees
- The New function's singleton behavior
- The instance interface requirements
+// shimLoaded ensures only one DNSListener instance is created var shimLoaded atomic.Bool +// New creates a singleton DNSListener instance. +// Returns an error if called multiple times. func New(instance instance) (*DNSListener, error) { +// instance defines the required capabilities for DNSListener integration. type instance interface {service/firewall/interception/dnslistener/eventlistener_linux.go (6)
21-21: Consider making the Varlink socket path configurable.The hardcoded path
/run/systemd/resolve/io.systemd.Resolve.Monitormight not be available on all Linux systems or configurations.Consider making it configurable:
+const defaultVarlinkSocket = "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor" + func newListener(module *DNSListener) (*Listener, error) { - varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor") + socketPath := module.config.GetString("varlink.socket", defaultVarlinkSocket) + varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), socketPath)
69-72: Consider removing unused flush method.The
flushmethod doesn't perform any operation and returns nil. If it's not required by an interface, consider removing it to reduce code clutter.
93-93: Fix typo in comment."Go trough" should be "Go through".
83-84: Document the magic number in slice capacity.The initial capacity of 5 for the IPs slice seems arbitrary. Consider adding a comment explaining why this value was chosen or make it a named constant.
+// typical DNS response rarely contains more than 5 IP addresses ips := make([]net.IP, 0, 5)
95-103: Add nil checks for RR fields.The code assumes that
a.RR.Addressanda.RR.Nameare safe to dereference when non-nil. Consider adding more defensive checks.- if a.RR.Address != nil { + if a.RR != nil && a.RR.Address != nil { ip := net.IP(*a.RR.Address) ips = append(ips, ip) - } else if a.RR.Name != nil { + } else if a.RR != nil && a.RR.Name != nil { cnames[domain] = *a.RR.Name }
106-106: Document the saveDomain function usage.Add a comment explaining what
saveDomaindoes and where it's defined, as it's not immediately clear from this file.+// saveDomain stores the resolved domain information in the DNS cache +// See module.go for implementation saveDomain(domain, ips, cnames)service/integration/etw_windows.go (1)
65-67: Include function names in error messages for clarityIn the error messages returned by methods like
InitializeSession,StartTrace,FlushTrace,StopTrace,DestroySession, andStopOldSession, consider including the function name in the error message to improve clarity during debugging.For example, modify the error messages as follows:
In
InitializeSession:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("InitializeSession failed with status code: %d", rc)In
StartTrace:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("StartTrace failed with status code: %d", rc)In
FlushTrace:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("FlushTrace failed with status code: %d", rc)In
StopTrace:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("StopTrace failed with status code: %d", rc)In
DestroySession:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("DestroySession failed with status code: %d", rc)In
StopOldSession:- return fmt.Errorf("failed with status code: %d", rc) + return fmt.Errorf("StopOldSession failed with status code: %d", rc)Also applies to: 73-75, 82-84, 91-93, 100-102, 109-111
service/network/connection.go (1)
548-560: Handle errors and add context in the retry mechanismAfter the retry loop, if the
errremains non-nil, consider handling the error or logging it for better debugging. Additionally, providing context with the error messages can aid in troubleshooting.Apply this diff to enhance error handling:
for i := 0; i < 4; i++ { _ = module.instance.DNSListener().Flush() ipinfo, err = resolver.GetIPInfo(resolver.IPInfoProfileScopeGlobal, pkt.Info().RemoteIP().String()) if err == nil { log.Tracer(pkt.Ctx()).Debugf("network: found domain from dnslistener after %d tries", i+1) break } time.Sleep(5 * time.Millisecond) } +if err != nil { + log.Tracer(pkt.Ctx()).Warningf("network: failed to resolve domain after retries: %v", err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
service/firewall/interception/dnslistener/etwlink_windows.go(1 hunks)service/firewall/interception/dnslistener/eventlistener.go(1 hunks)service/firewall/interception/dnslistener/eventlistener_linux.go(1 hunks)service/firewall/interception/dnslistener/eventlistener_windows.go(1 hunks)service/firewall/interception/dnslistener/module.go(1 hunks)service/instance.go(9 hunks)service/integration/etw_windows.go(1 hunks)service/integration/integration.go(1 hunks)service/integration/integration_windows.go(1 hunks)service/integration/module.go(1 hunks)service/network/connection.go(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- service/integration/integration.go
🚧 Files skipped from review as they are similar to previous changes (1)
- service/firewall/interception/dnslistener/eventlistener.go
🧰 Additional context used
📓 Learnings (1)
service/firewall/interception/dnslistener/etwlink_windows.go (1)
Learnt from: vlabo
PR: safing/portmaster#1728
File: service/firewall/interception/dnslistener/etwlink_windows.go:71-74
Timestamp: 2024-11-18T14:08:51.570Z
Learning: In the file `service/firewall/interception/dnslistener/etwlink_windows.go`, the developer is explicitly calling `DestroySession` and is using `runtime.SetFinalizer` as an additional safety net to enhance resilience in case `DestroySession` is not called due to a panic or error.
🔇 Additional comments (15)
service/integration/integration_windows.go (1)
6-11: LGTM!
The imports are appropriate and necessary for the Windows-specific functionality.
service/integration/module.go (1)
1-9: LGTM! Clean package structure and imports.
The package name and imports are well-organized and appropriate for the module's functionality.
service/firewall/interception/dnslistener/eventlistener_windows.go (4)
1-4: LGTM: Build tags are properly configured.
The build constraints are correctly formatted and include both old and new style tags for maximum compatibility.
6-14: LGTM: Imports are well-organized.
The imports are properly grouped and all are necessary for the implementation.
16-18: Verify ETWSession type definition.
The struct looks good, but let's verify the ETWSession type definition.
✅ Verification successful
Let me try another search strategy to locate the ETWSession type definition.
ETWSession type is properly defined and implemented
The ETWSession type is well-defined in service/firewall/interception/dnslistener/etwlink_windows.go. It's a Windows-specific implementation that includes:
- Integration with ETW functions interface
- Proper shutdown handling with atomic flags and mutex
- Complete lifecycle management (initialization, start, stop, destroy)
- Memory management with runtime finalizer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for ETWSession type definition
ast-grep --pattern 'type ETWSession struct {
$$$
}'
# Backup search using ripgrep
rg "type ETWSession"
Length of output: 254
Script:
#!/bin/bash
# Search for ETWSession in the entire codebase
rg "ETWSession" -A 5
# Also search for files containing ETW
fd -e go -e h -e c "ETW"
Length of output: 4683
93-93: Verify saveDomain function definition and error handling.
The saveDomain function is called but not defined in this file. Let's verify its implementation and error handling.
✅ Verification successful
Error handling is properly implemented in saveDomain function
The saveDomain function is defined in module.go and includes appropriate error handling. It:
- Properly checks and skips localhost IPs
- Has error handling for the
Save()operation with logging - Uses appropriate domain record structures with CNAME handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for saveDomain function definition
ast-grep --pattern 'func saveDomain($domain string, $ips []net.IP, $cnames map[string]string) {
$$$
}'
# Backup search using ripgrep
rg "func saveDomain"
Length of output: 254
Script:
#!/bin/bash
# Get the implementation of saveDomain function with context
rg -A 20 "func saveDomain" service/firewall/interception/dnslistener/module.go
Length of output: 633
Script:
#!/bin/bash
# Get more context of the saveDomain function implementation
rg -A 30 "func saveDomain" service/firewall/interception/dnslistener/module.go
# Check if there's any error handling in the module
rg "resolver\.SaveResolvedDomain|resolver\.Save" service/firewall/interception/dnslistener/module.go
Length of output: 941
Script:
#!/bin/bash
# Get the complete implementation including where the record is saved
rg -B 5 -A 40 "func saveDomain" service/firewall/interception/dnslistener/module.go
# Also check for any resolver package usage
rg "resolver\." service/firewall/interception/dnslistener/module.go
Length of output: 1427
service/firewall/interception/dnslistener/etwlink_windows.go (3)
1-20: LGTM! Well-structured type definition with proper synchronization.
The ETWSession struct is well-designed with:
- Thread-safe shutdown guard using atomic operations
- Proper synchronization using mutex
- Clean interface separation for ETW functions
32-35: Ensure the callback function is not garbage collected prematurely.
81-98: Verify ETW session cleanup on process termination.
While the code handles cleanup well during normal operation, we should verify that ETW sessions are properly cleaned up if the process is terminated unexpectedly.
✅ Verification successful
Let me analyze the code further to understand the ETW session cleanup mechanisms.
ETW session cleanup is properly handled
The code has robust cleanup mechanisms in place:
- A runtime finalizer ensures ETW sessions are destroyed even if
DestroySession()isn't explicitly called StopOldSession()is called during initialization to clean up any lingering sessions- The
DestroySession()method is thread-safe with mutex and atomic guards
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any leaked ETW sessions from previous runs
# Note: This would require access to Windows ETW APIs which aren't available in the sandbox
# The following is a pseudo-command showing what we'd want to verify:
# Get-EtwTraceSession | Where-Object { $_.LoggerName -like "*portmaster*" }
# Instead, we can look for any cleanup-related code in the codebase
rg -A 5 "StopOldSession|DestroySession" --type go
Length of output: 5455
Script:
#!/bin/bash
# Check for any finalizer or cleanup related code
rg -A 5 "SetFinalizer|runtime\.SetFinalizer" --type go
# Look for any init or startup code that might handle old sessions
rg -A 5 "NewETWSession|InitializeSession" --type go
Length of output: 3174
service/firewall/interception/dnslistener/eventlistener_linux.go (3)
1-14: LGTM! Build tags and imports are properly structured.
The build constraints and imports are well-organized and necessary for the functionality.
15-18: LGTM! Clean and focused struct definition.
The Listener struct is well-defined with a clear purpose.
41-41: Duplicate Comment: Undefined type QueryResult.
This issue was already identified in a previous review. Please ensure the QueryResult type is properly defined or imported.
service/instance.go (1)
70-70: Verify systemd integration compatibility
The addition of integration and dnslistener modules suggests integration with systemd. Please ensure:
- The systemd version compatibility requirements are documented
- The integration gracefully handles systems without systemd
Also applies to: 80-80
✅ Verification successful
Let's search for more concrete evidence of systemd integration and error handling patterns in the codebase. This will help us verify:
- How systemd integration is implemented
- Whether there's proper error handling
- If there's any documentation about systemd requirements
Let's search for more specific evidence of systemd integration implementation and error handling to verify:
- How the code handles systems without systemd
- Whether there's proper error handling for systemd integration
- If there's documentation about systemd requirements
Based on the search results, I can now generate a final response:
Systemd integration is properly handled and platform-specific
The systemd integration is:
- Only used on Linux and properly isolated in platform-specific files (
*_linux.go) - Handles errors appropriately with detailed error messages
- Implements graceful fallbacks through OS-specific interfaces
Key evidence:
- The
OSIntegrationinterface provides platform abstraction - Linux-specific systemd code is isolated in
eventlistener_linux.go - Windows has its own implementation in
integration_windows.go - Error handling includes specific messages for systemd connection failures
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for systemd version requirements or compatibility documentation
rg -l 'systemd.*version|version.*systemd'
# Check for systemd availability handling
ast-grep --pattern 'if err := systemd.$_(); err != nil {
$$$
}'
Length of output: 111
Script:
#!/bin/bash
# Search for systemd related code and error handling patterns
rg -A 5 'systemd|osintegration' --type go
# Look for any documentation files mentioning systemd
fd -e md -e txt -X rg -l 'systemd'
# Check for integration error handling patterns
ast-grep --pattern 'if $_ == nil {
$$$
}'
Length of output: 118470
Script:
#!/bin/bash
# Search for systemd-related error handling and availability checks
rg -A 5 'systemd.*err|if.*systemd|systemd.*available' --type go
# Look for systemd integration implementation
rg -A 10 'OSIntegration|integration.*systemd' --type go
# Check for any systemd-related documentation
fd -e md -X rg -l 'systemd'
Length of output: 7766
service/network/connection.go (2)
8-8: Ensure the runtime package is necessary
Confirm that the runtime package is required. If it's only used for runtime.GOOS, consider using the standard os package instead.
Run the following script to check the usage of runtime.GOOS:
✅ Verification successful
Let me try a different approach to verify the usage of the runtime package.
Based on the code analysis, I can now provide a final response regarding the usage of the runtime package.
The runtime package is necessary in this file
The runtime package is used for Windows-specific domain resolution handling, which is a critical platform-specific functionality. The usage at line 1012 checks the operating system to implement special handling for delayed domain resolution on Windows systems.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify if only `runtime.GOOS` is used from the `runtime` package.
# Search for `runtime.GOOS` usage
rg 'runtime\.GOOS'
Length of output: 1281
Script:
#!/bin/bash
# Search for all runtime package usages in the codebase
rg 'runtime\.' -A 2
Length of output: 95950
Script:
#!/bin/bash
# Check the specific file for runtime usage
cat service/network/connection.go
Length of output: 31106
22-22: Verify the necessity of the reference package import
Ensure that all usages of the reference package are intentional and required.
Run the following script to find references to the reference package:
✅ Verification successful
Let me verify the usage of the reference package in the specific file and its dependencies.
Let me check the actual reference package content to understand its usage.
Let me check the content of these reference package files to understand their purpose and usage.
Based on the analysis of the code, I can now generate the final response.
The reference package import is necessary and correctly used
The reference package provides essential protocol and port mapping utilities that are actively used in connection.go. Specifically:
reference.IsICMP()is used to check if a connection uses ICMP protocol in theshouldWaitForDomain()method- The package provides critical functionality for mapping between protocol numbers and names, port numbers and names
- It's also used by other files in the codebase (
profile/endpoints/endpoint.goandprocess/find.go) for similar protocol and port reference purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find usages of the `reference` package.
# Search for `reference.` usage
rg 'reference\.'
Length of output: 1281
Script:
#!/bin/bash
# Check the content of the connection.go file to understand the context
cat service/network/connection.go
Length of output: 31106
Script:
#!/bin/bash
# Check if there are any other imports of the reference package
rg --type go "\"github.com/safing/portmaster/service/network/reference\""
Length of output: 339
Script:
#!/bin/bash
# Check the reference package itself
fd reference.go -e go
Length of output: 23
Script:
#!/bin/bash
# Find and check the reference package files
rg --type go -l "package reference"
Length of output: 111
Script:
#!/bin/bash
# Search for the actual implementation files in the reference package directory
ls -la service/network/reference/
Length of output: 266
Script:
#!/bin/bash
# Check the content of the reference package files
cat service/network/reference/ports.go service/network/reference/protocols.go
Length of output: 3427
| func (i *OSIntegration) Initialize() error { | ||
| // Find path to the dll. | ||
| file, err := updates.GetFile("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) | ||
| } | ||
|
|
||
| // Enumerate all needed dll functions. | ||
| i.os.etwFunctions, err = initializeETW(i.os.dll) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the investigation of the codebase, I can now generate the final response:
Add cleanup handling for DLL loading
The review comment is partially correct. While the DLL path validation is not strictly necessary since it comes from a trusted source (updates package), cleanup handling should be added to prevent resource leaks:
func (i *OSIntegration) Initialize() error {
// Find path to the dll.
file, err := updates.GetFile("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)
}
+ // Ensure cleanup on failure
+ var success bool
+ defer func() {
+ if !success {
+ 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
}
+ success = true
return nil
}The other suggestions from the review are not necessary because:
- The DLL path comes from the updates package which handles file integrity and security
- The updates package already validates and manages file paths internally
🔗 Analysis chain
Add cleanup handling and validate DLL path.
The initialization process has several potential issues that need to be addressed:
- There's no cleanup if initialization fails after the DLL is loaded
- The DLL path should be validated before loading
- Loading a DLL from the updates package has security implications
Here's a suggested improvement:
func (i *OSIntegration) Initialize() error {
// Find path to the dll.
file, err := updates.GetFile("portmaster-core.dll")
if err != nil {
return err
}
+ // Validate DLL path
+ if err := validateDLLPath(file.Path()); err != nil {
+ return fmt.Errorf("invalid DLL path: %w", err)
+ }
+
// Load the DLL.
i.os.dll, err = windows.LoadDLL(file.Path())
if err != nil {
return fmt.Errorf("failed to load dll: %q", err)
}
+ // Ensure cleanup on failure
+ defer func() {
+ if err != 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
}
return nil
}Let's verify the DLL loading security:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check DLL loading security implications
# Check if the DLL is loaded from a secure location
rg -l "updates.GetFile.*portmaster-core.dll"
# Check for any validation of DLL paths
rg -l "validateDLL|DLLPath|PathValidation"
# Look for any security-related comments or documentation
rg -l "SECURITY:|security consideration|secure path"
Length of output: 189
Script:
#!/bin/bash
# Let's investigate the updates package and DLL handling
# Check the updates package implementation
rg -A 5 "GetFile" --type go
# Check for any DLL release or cleanup
rg -A 5 "Release|Unload|Close" --type go
# Look for any existing error handling patterns with DLLs
ast-grep --pattern 'if err := $_.LoadDLL($$$); err != nil { $$$ }'
# Check the integration struct and its cleanup methods
rg -A 10 "type OSIntegration struct" --type go
rg -A 10 "func.*Shutdown|func.*Close|func.*cleanup" --type go
Length of output: 186282
service/integration/module.go
Outdated
| var shimLoaded atomic.Bool | ||
|
|
||
| // New returns a new UI module. | ||
| func New(instance instance) (*OSIntegration, error) { | ||
| if !shimLoaded.CompareAndSwap(false, true) { | ||
| return nil, errors.New("only one instance allowed") | ||
| } | ||
|
|
||
| m := mgr.New("OSIntegration") | ||
| module := &OSIntegration{ | ||
| m: m, | ||
| states: m.NewStateMgr(), | ||
|
|
||
| instance: instance, | ||
| } | ||
|
|
||
| return module, nil | ||
| } |
There was a problem hiding this comment.
Initialize the 'os' field to prevent nil pointer dereference.
The os field of type OSSpecific is left uninitialized in the constructor. This could lead to runtime panics if the field is accessed before being properly set.
Consider one of these approaches:
- Initialize it in the constructor if possible
- Add validation in methods that use this field
- Document when and where this field should be initialized
Also, the comment for New appears to be incorrect:
-// New returns a new UI module.
+// New returns a new OSIntegration module.Committable suggestion skipped: line range outside the PR's diff.
service/integration/module.go
Outdated
| // Start starts the module. | ||
| func (i *OSIntegration) Start() error { | ||
| return i.Initialize() | ||
| } | ||
|
|
||
| // Stop stops the module. | ||
| func (i *OSIntegration) Stop() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
DLL resource leak in Stop() method
The verification reveals that:
Initialize()is properly implemented in both platforms:- Default implementation returns nil
- Windows implementation loads a DLL and initializes ETW functions
Stop()is incomplete as it doesn't properly clean up the loaded DLL on Windows
service/integration/module.go: Add DLL cleanup inStop()method to prevent resource leaks
🔗 Analysis chain
Verify Initialize() implementation and Stop() completeness.
Two potential issues:
Start()callsInitialize()which is not defined in this fileStop()is a no-op, which might not properly clean up resources
Let me gather more information about the Initialize implementation and potential cleanup needs.
Let me check one more thing to understand if there are any DLL or system resources that need cleanup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Initialize() implementation
echo "Searching for Initialize() implementation:"
ast-grep --pattern 'func (*OSIntegration) Initialize() error'
# Search for potential cleanup needs
echo "Checking for resources that might need cleanup:"
rg -l "OSSpecific" --type go
Length of output: 400
Script:
#!/bin/bash
# Check Initialize implementation in other files
echo "Checking Initialize implementation in integration.go:"
rg -A 5 "Initialize\(\)" service/integration/integration.go
echo "Checking Initialize implementation in integration_windows.go:"
rg -A 5 "Initialize\(\)" service/integration/integration_windows.go
# Check for resource allocations or setup that might need cleanup
echo "Checking for resource allocations or setup:"
rg -A 5 "New|Create|Open|Start" service/integration/module.go service/integration/integration.go service/integration/integration_windows.go
Length of output: 1993
Script:
#!/bin/bash
# Check for any DLL or system resource handling
echo "Checking for DLL and system resource handling:"
rg -A 10 "dll|handle|syscall" service/integration/integration_windows.go
# Check if there are any cleanup functions defined
echo "Checking for cleanup functions:"
rg -A 5 "cleanup|close|unload|release" service/integration/integration_windows.go service/integration/integration.go
Length of output: 1089
| func (l *Listener) processEvent(domain string, result string) { | ||
| // Ignore empty results | ||
| if len(result) == 0 { | ||
| return | ||
| } | ||
|
|
||
| cnames := make(map[string]string) | ||
| ips := []net.IP{} | ||
|
|
||
| resultArray := strings.Split(result, ";") | ||
| for _, r := range resultArray { | ||
| // For result different then IP the string starts with "type:" | ||
| if strings.HasPrefix(r, "type:") { | ||
| dnsValueArray := strings.Split(r, " ") | ||
| if len(dnsValueArray) < 3 { | ||
| continue | ||
| } | ||
|
|
||
| // Ignore evrything else exept CNAME. | ||
| if value, err := strconv.ParseInt(dnsValueArray[1], 10, 16); err == nil && value == int64(dns.TypeCNAME) { | ||
| cnames[domain] = dnsValueArray[2] | ||
| } | ||
|
|
||
| } else { | ||
| // The events deosn't start with "type:" that means it's an IP address. | ||
| ip := net.ParseIP(r) | ||
| if ip != nil { | ||
| ips = append(ips, ip) | ||
| } | ||
| } | ||
| } | ||
| saveDomain(domain, ips, cnames) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Multiple improvements needed in processEvent method.
- The method lacks input validation for the domain parameter.
- String parsing is fragile and could benefit from more robust parsing.
- Comments contain typos ("evrything", "exept", "deosn't").
func (l *Listener) processEvent(domain string, result string) {
+ // Validate domain parameter
+ if domain == "" {
+ return
+ }
+
// Ignore empty results
if len(result) == 0 {
return
}
cnames := make(map[string]string)
ips := []net.IP{}
resultArray := strings.Split(result, ";")
for _, r := range resultArray {
- // For result different then IP the string starts with "type:"
+ // For results other than IP addresses, the string starts with "type:"
if strings.HasPrefix(r, "type:") {
dnsValueArray := strings.Split(r, " ")
if len(dnsValueArray) < 3 {
continue
}
- // Ignore evrything else exept CNAME.
+ // Ignore everything except CNAME records
if value, err := strconv.ParseInt(dnsValueArray[1], 10, 16); err == nil && value == int64(dns.TypeCNAME) {
cnames[domain] = dnsValueArray[2]
}
} else {
- // The events deosn't start with "type:" that means it's an IP address.
+ // If the event doesn't start with "type:", it's an IP address
ip := net.ParseIP(r)
if ip != nil {
ips = append(ips, ip)
}
}
}
saveDomain(domain, ips, cnames)
}📝 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 (l *Listener) processEvent(domain string, result string) { | |
| // Ignore empty results | |
| if len(result) == 0 { | |
| return | |
| } | |
| cnames := make(map[string]string) | |
| ips := []net.IP{} | |
| resultArray := strings.Split(result, ";") | |
| for _, r := range resultArray { | |
| // For result different then IP the string starts with "type:" | |
| if strings.HasPrefix(r, "type:") { | |
| dnsValueArray := strings.Split(r, " ") | |
| if len(dnsValueArray) < 3 { | |
| continue | |
| } | |
| // Ignore evrything else exept CNAME. | |
| if value, err := strconv.ParseInt(dnsValueArray[1], 10, 16); err == nil && value == int64(dns.TypeCNAME) { | |
| cnames[domain] = dnsValueArray[2] | |
| } | |
| } else { | |
| // The events deosn't start with "type:" that means it's an IP address. | |
| ip := net.ParseIP(r) | |
| if ip != nil { | |
| ips = append(ips, ip) | |
| } | |
| } | |
| } | |
| saveDomain(domain, ips, cnames) | |
| } | |
| func (l *Listener) processEvent(domain string, result string) { | |
| // Validate domain parameter | |
| if domain == "" { | |
| return | |
| } | |
| // Ignore empty results | |
| if len(result) == 0 { | |
| return | |
| } | |
| cnames := make(map[string]string) | |
| ips := []net.IP{} | |
| resultArray := strings.Split(result, ";") | |
| for _, r := range resultArray { | |
| // For results other than IP addresses, the string starts with "type:" | |
| if strings.HasPrefix(r, "type:") { | |
| dnsValueArray := strings.Split(r, " ") | |
| if len(dnsValueArray) < 3 { | |
| continue | |
| } | |
| // Ignore everything except CNAME records | |
| if value, err := strconv.ParseInt(dnsValueArray[1], 10, 16); err == nil && value == int64(dns.TypeCNAME) { | |
| cnames[domain] = dnsValueArray[2] | |
| } | |
| } else { | |
| // If the event doesn't start with "type:", it's an IP address | |
| ip := net.ParseIP(r) | |
| if ip != nil { | |
| ips = append(ips, ip) | |
| } | |
| } | |
| } | |
| saveDomain(domain, ips, cnames) | |
| } |
| // Flush flushes the buffer forcing all events to be processed. | ||
| func (dl *DNSListener) Flush() error { | ||
| return dl.listener.flush() | ||
| } |
There was a problem hiding this comment.
Add nil check in Flush method
The Flush method should check if the listener is nil before calling flush() to prevent panic.
func (dl *DNSListener) Flush() error {
+ if dl.listener == nil {
+ return errors.New("listener not initialized")
+ }
return dl.listener.flush()
}📝 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.
| // Flush flushes the buffer forcing all events to be processed. | |
| func (dl *DNSListener) Flush() error { | |
| return dl.listener.flush() | |
| } | |
| // Flush flushes the buffer forcing all events to be processed. | |
| func (dl *DNSListener) Flush() error { | |
| if dl.listener == nil { | |
| return errors.New("listener not initialized") | |
| } | |
| return dl.listener.flush() | |
| } |
service/integration/etw_windows.go
Outdated
| var err error | ||
| functions.createState, err = dll.FindProc("PM_ETWCreateState") | ||
| if err != nil { | ||
| return functions, fmt.Errorf("faild to load function PM_ETWCreateState: %q", err) |
There was a problem hiding this comment.
Correct spelling in error messages: "faild" should be "failed"
In the error messages within the initializeETW function, the word "failed" is misspelled as "faild". Please correct the spelling to ensure accurate error reporting.
Apply this diff to fix the spelling errors:
- return functions, fmt.Errorf("faild to load function PM_ETWCreateState: %q", err)
+ return functions, fmt.Errorf("failed to load function PM_ETWCreateState: %q", err)
- return functions, fmt.Errorf("faild to load function PM_ETWInitializeSession: %q", err)
+ return functions, fmt.Errorf("failed to load function PM_ETWInitializeSession: %q", err)
- return functions, fmt.Errorf("faild to load function PM_ETWStartTrace: %q", err)
+ return functions, fmt.Errorf("failed to load function PM_ETWStartTrace: %q", err)
- return functions, fmt.Errorf("faild to load function PM_ETWFlushTrace: %q", err)
+ return functions, fmt.Errorf("failed to load function PM_ETWFlushTrace: %q", err)
- return functions, fmt.Errorf("faild to load function PM_ETWStopTrace: %q", err)
+ return functions, fmt.Errorf("failed to load function PM_ETWStopTrace: %q", err)
- return functions, fmt.Errorf("faild to load function PM_ETWDestroySession: %q", err)
+ return functions, fmt.Errorf("failed to load function PM_ETWDestroySession: %q", err)Also applies to: 31-31, 35-35, 39-39, 43-43, 47-47
service/integration/etw_windows.go
Outdated
| } | ||
| functions.stopOldSession, err = dll.FindProc("PM_ETWStopOldSession") | ||
| if err != nil { | ||
| return functions, fmt.Errorf("faild to load function PM_ETWDestroySession: %q", err) |
There was a problem hiding this comment.
Fix incorrect function name and spelling in error message
In the error message on line 51, the function name is incorrect and there's a spelling mistake. It should be "PM_ETWStopOldSession" instead of "PM_ETWDestroySession", and "faild" should be "failed".
Apply this diff to correct the function name and spelling:
- return functions, fmt.Errorf("faild to load function PM_ETWDestroySession: %q", err)
+ return functions, fmt.Errorf("failed to load function PM_ETWStopOldSession: %q", err)📝 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.
| return functions, fmt.Errorf("faild to load function PM_ETWDestroySession: %q", err) | |
| return functions, fmt.Errorf("failed to load function PM_ETWStopOldSession: %q", err) |
| func (etw ETWFunctions) CreateState(callback uintptr) uintptr { | ||
| state, _, _ := etw.createState.Call(callback) | ||
| return state | ||
| } |
There was a problem hiding this comment.
Add error handling in CreateState method
The CreateState method does not check for errors returned by the createState call. This could lead to unhandled errors if the function fails. It's important to verify if the state was created successfully.
Apply this diff to add error handling:
-func (etw ETWFunctions) CreateState(callback uintptr) uintptr {
- state, _, _ := etw.createState.Call(callback)
- return state
+func (etw ETWFunctions) CreateState(callback uintptr) (uintptr, error) {
+ state, _, _ := etw.createState.Call(callback)
+ if state == 0 {
+ return 0, fmt.Errorf("CreateState failed to create state")
+ }
+ return state, nil
}Note: Update the calling code to handle the additional error returned by this method.
📝 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 (etw ETWFunctions) CreateState(callback uintptr) uintptr { | |
| state, _, _ := etw.createState.Call(callback) | |
| return state | |
| } | |
| func (etw ETWFunctions) CreateState(callback uintptr) (uintptr, error) { | |
| state, _, _ := etw.createState.Call(callback) | |
| if state == 0 { | |
| return 0, fmt.Errorf("CreateState failed to create state") | |
| } | |
| return state, nil | |
| } |
| // On windows domains may come with delay. | ||
| if module.instance.Resolver().IsDisabled.IsSet() && conn.shouldWaitForDomain() { | ||
| // Flush the dns listener buffer and try again. | ||
| for i := range 4 { |
There was a problem hiding this comment.
Fix incorrect loop syntax in retry mechanism
The loop for i := range 4 is not valid Go syntax because you cannot range over an integer. To loop a fixed number of times, use a traditional for loop.
Apply this diff to fix the loop syntax:
-for i := range 4 {
+for i := 0; i < 4; i++ {📝 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.
| for i := range 4 { | |
| for i := 0; i < 4; i++ { |
service/network/connection.go
Outdated
| if conn.Entity.IPScope == netutils.Global && | ||
| !conn.Inbound && | ||
| reference.IsICMP(conn.Entity.Protocol) { |
There was a problem hiding this comment.
Correct logical condition in shouldWaitForDomain method
The shouldWaitForDomain function currently returns true when the protocol is ICMP, which contradicts the intention stated in the comment: "Should wait for Global Unicast, outgoing not ICMP connections." The condition should check for non-ICMP protocols.
Apply this diff to fix the condition:
if conn.Entity.IPScope == netutils.Global &&
!conn.Inbound &&
- reference.IsICMP(conn.Entity.Protocol) {
+ !reference.IsICMP(conn.Entity.Protocol) {
return true
}📝 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.
| if conn.Entity.IPScope == netutils.Global && | |
| !conn.Inbound && | |
| reference.IsICMP(conn.Entity.Protocol) { | |
| if conn.Entity.IPScope == netutils.Global && | |
| !conn.Inbound && | |
| !reference.IsICMP(conn.Entity.Protocol) { |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
windows_core_dll/build.ps1 (1)
1-2: Consider adding configuration flexibility and documentationThe script could benefit from additional features for better maintainability and flexibility.
Consider these additional improvements:
+[CmdletBinding()] +param( + [ValidateSet('Release', 'Debug')] + [string]$Configuration = 'Release', + + [ValidateSet('x64', 'Win32')] + [string]$Platform = 'x64' +) + +# Script documentation +<# +.SYNOPSIS + Builds the portmaster-core.dll Windows component. +.DESCRIPTION + Compiles the Windows core DLL that implements ETW session management for DNS events. +.PARAMETER Configuration + Build configuration: 'Release' or 'Debug'. Defaults to 'Release'. +.PARAMETER Platform + Target platform: 'x64' or 'Win32'. Defaults to 'x64'. +#> + +# Build command with flexible configuration +$buildArgs = @( + ".\windows_core_dll.sln", + "/p:Configuration=$Configuration", + "/p:Platform=$Platform" +) + msbuild .\windows_core_dll.sln /p:Configuration=Release -ls .\x64\Release\portmaster-core.dll + +# Output path based on configuration +$outputPath = ".\$Platform\$Configuration\portmaster-core.dll"windows_core_dll/dllmain.cpp (2)
164-174: Correct Function Comment and Spelling Errors inPM_ETWDestroySessionThe comment for
PM_ETWDestroySessionrefers toPM_ETWFlushTraceand contains a spelling error. Accurate comments improve code readability.Apply this diff:
-// PM_ETWFlushTrace Closes the session and frees resourses. +// PM_ETWDestroySession closes the session and frees resources. __declspec(dllexport) uint32_t PM_ETWDestroySession(ETWSessionState* state) { if (state == NULL) { return 1; } uint32_t status = CloseTrace(state->sessionHandle); // Free memory. free(state->SessionProperties); free(state); return status; }
1-197: Unit Tests Needed for ETW Session FunctionsThe new ETW session management functions lack unit tests. Adding unit tests will help ensure their reliability and make future maintenance easier.
Would you like assistance in creating unit tests for these functions?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
windows_core_dll/build.ps1(1 hunks)windows_core_dll/dllmain.cpp(1 hunks)windows_core_dll/framework.h(1 hunks)windows_core_dll/pch.cpp(1 hunks)windows_core_dll/pch.h(1 hunks)windows_core_dll/windows_core_dll.sln(1 hunks)windows_core_dll/windows_core_dll.vcxproj(1 hunks)windows_core_dll/windows_core_dll.vcxproj.filters(1 hunks)windows_core_dll/windows_core_dll.vcxproj.user(1 hunks)
✅ Files skipped from review due to trivial changes (7)
- windows_core_dll/framework.h
- windows_core_dll/pch.cpp
- windows_core_dll/pch.h
- windows_core_dll/windows_core_dll.sln
- windows_core_dll/windows_core_dll.vcxproj
- windows_core_dll/windows_core_dll.vcxproj.filters
- windows_core_dll/windows_core_dll.vcxproj.user
🔇 Additional comments (2)
windows_core_dll/dllmain.cpp (2)
25-33: Consider 32-bit Compatibility for Callback Function Pointer
The GoEventRecordCallback typedef uses uint64_t as the return type, which might cause issues on 32-bit systems. Ensure that the code is compatible with both 32-bit and 64-bit architectures.
158-161: 🛠️ Refactor suggestion
Incorrect Function Name in Comment
The comment for PM_ETWStopTrace incorrectly refers to PM_ETWFlushTrace. This could cause confusion when maintaining the code.
Apply this diff to correct the comment:
-// PM_ETWFlushTrace stops the listener.
+// PM_ETWStopTrace stops the listener.
__declspec(dllexport) uint32_t PM_ETWStopTrace(ETWSessionState* state) {
return ControlTrace(state->SessionTraceHandle, LOGSESSION_NAME, state->SessionProperties,
EVENT_TRACE_CONTROL_STOP);
}Likely invalid or redundant comment.
| msbuild .\windows_core_dll.sln /p:Configuration=Release | ||
| ls .\x64\Release\portmaster-core.dll |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add essential error handling and build validation
The current build script lacks error handling and proper build validation.
Consider implementing these essential improvements:
-msbuild .\windows_core_dll.sln /p:Configuration=Release
-ls .\x64\Release\portmaster-core.dll
+# Ensure MSBuild is available
+if (-not (Get-Command msbuild -ErrorAction SilentlyContinue)) {
+ throw "MSBuild is not available in the path"
+}
+
+# Clean previous builds
+Remove-Item -Path ".\x64\Release\*" -Force -ErrorAction SilentlyContinue
+
+# Build with error handling
+$buildResult = msbuild .\windows_core_dll.sln /p:Configuration=Release
+if ($LASTEXITCODE -ne 0) {
+ throw "Build failed with exit code $LASTEXITCODE"
+}
+
+# Verify DLL exists and basic properties
+$dllPath = ".\x64\Release\portmaster-core.dll"
+if (-not (Test-Path $dllPath)) {
+ throw "Build output not found: $dllPath"
+}
+
+Write-Host "Build completed successfully. Output file details:"
+Get-Item $dllPath | Select-Object Name, Length, LastWriteTime📝 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.
| msbuild .\windows_core_dll.sln /p:Configuration=Release | |
| ls .\x64\Release\portmaster-core.dll | |
| # Ensure MSBuild is available | |
| if (-not (Get-Command msbuild -ErrorAction SilentlyContinue)) { | |
| throw "MSBuild is not available in the path" | |
| } | |
| # Clean previous builds | |
| Remove-Item -Path ".\x64\Release\*" -Force -ErrorAction SilentlyContinue | |
| # Build with error handling | |
| $buildResult = msbuild .\windows_core_dll.sln /p:Configuration=Release | |
| if ($LASTEXITCODE -ne 0) { | |
| throw "Build failed with exit code $LASTEXITCODE" | |
| } | |
| # Verify DLL exists and basic properties | |
| $dllPath = ".\x64\Release\portmaster-core.dll" | |
| if (-not (Test-Path $dllPath)) { | |
| throw "Build output not found: $dllPath" | |
| } | |
| Write-Host "Build completed successfully. Output file details:" | |
| Get-Item $dllPath | Select-Object Name, Length, LastWriteTime |
| __declspec(dllexport) uint32_t PM_ETWInitializeSession(ETWSessionState* state) { | ||
| return StartTrace(&state->SessionTraceHandle, LOGSESSION_NAME, | ||
| state->SessionProperties); | ||
| } |
There was a problem hiding this comment.
Null Pointer Check Needed in PM_ETWInitializeSession
The function PM_ETWInitializeSession does not verify if the state pointer or state->SessionProperties is NULL before using them. Dereferencing a NULL pointer will lead to a crash.
Apply this diff to add null checks:
__declspec(dllexport) uint32_t PM_ETWInitializeSession(ETWSessionState* state) {
+ if (state == NULL || state->SessionProperties == NULL) {
+ return ERROR_INVALID_PARAMETER;
+ }
return StartTrace(&state->SessionTraceHandle, LOGSESSION_NAME,
state->SessionProperties);
}📝 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.
| __declspec(dllexport) uint32_t PM_ETWInitializeSession(ETWSessionState* state) { | |
| return StartTrace(&state->SessionTraceHandle, LOGSESSION_NAME, | |
| state->SessionProperties); | |
| } | |
| __declspec(dllexport) uint32_t PM_ETWInitializeSession(ETWSessionState* state) { | |
| if (state == NULL || state->SessionProperties == NULL) { | |
| return ERROR_INVALID_PARAMETER; | |
| } | |
| return StartTrace(&state->SessionTraceHandle, LOGSESSION_NAME, | |
| state->SessionProperties); | |
| } |
| static void WINAPI EventRecordCallback(PEVENT_RECORD eventRecord) { | ||
| PBYTE resultValue = NULL; | ||
| PBYTE domainValue = NULL; | ||
|
|
||
| getPropertyValue(eventRecord, (LPWSTR)L"QueryResults", &resultValue); | ||
| getPropertyValue(eventRecord, (LPWSTR)L"QueryName", &domainValue); | ||
|
|
||
| ETWSessionState* state = (ETWSessionState*)eventRecord->UserContext; | ||
|
|
||
| if (resultValue != NULL && domainValue != NULL) { | ||
| state->callback((wchar_t*)domainValue, (wchar_t*)resultValue); | ||
| } | ||
|
|
||
| free(resultValue); | ||
| free(domainValue); | ||
| } |
There was a problem hiding this comment.
Check for Null Pointers in EventRecordCallback
In EventRecordCallback, the eventRecord->UserContext is cast to ETWSessionState* without verifying if it is NULL. This could lead to a crash if UserContext is not set correctly.
Apply this diff to add null checks:
static void WINAPI EventRecordCallback(PEVENT_RECORD eventRecord) {
PBYTE resultValue = NULL;
PBYTE domainValue = NULL;
getPropertyValue(eventRecord, (LPWSTR)L"QueryResults", &resultValue);
getPropertyValue(eventRecord, (LPWSTR)L"QueryName", &domainValue);
+ if (eventRecord->UserContext == NULL) {
+ // Cannot proceed without a valid state
+ return;
+ }
ETWSessionState* state = (ETWSessionState*)eventRecord->UserContext;
+ if (state->callback == NULL) {
+ // Cannot call a NULL callback
+ return;
+ }
if (resultValue != NULL && domainValue != NULL) {
state->callback((wchar_t*)domainValue, (wchar_t*)resultValue);
}
free(resultValue);
free(domainValue);
}📝 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.
| static void WINAPI EventRecordCallback(PEVENT_RECORD eventRecord) { | |
| PBYTE resultValue = NULL; | |
| PBYTE domainValue = NULL; | |
| getPropertyValue(eventRecord, (LPWSTR)L"QueryResults", &resultValue); | |
| getPropertyValue(eventRecord, (LPWSTR)L"QueryName", &domainValue); | |
| ETWSessionState* state = (ETWSessionState*)eventRecord->UserContext; | |
| if (resultValue != NULL && domainValue != NULL) { | |
| state->callback((wchar_t*)domainValue, (wchar_t*)resultValue); | |
| } | |
| free(resultValue); | |
| free(domainValue); | |
| } | |
| static void WINAPI EventRecordCallback(PEVENT_RECORD eventRecord) { | |
| PBYTE resultValue = NULL; | |
| PBYTE domainValue = NULL; | |
| getPropertyValue(eventRecord, (LPWSTR)L"QueryResults", &resultValue); | |
| getPropertyValue(eventRecord, (LPWSTR)L"QueryName", &domainValue); | |
| if (eventRecord->UserContext == NULL) { | |
| // Cannot proceed without a valid state | |
| return; | |
| } | |
| ETWSessionState* state = (ETWSessionState*)eventRecord->UserContext; | |
| if (state->callback == NULL) { | |
| // Cannot call a NULL callback | |
| return; | |
| } | |
| if (resultValue != NULL && domainValue != NULL) { | |
| state->callback((wchar_t*)domainValue, (wchar_t*)resultValue); | |
| } | |
| free(resultValue); | |
| free(domainValue); | |
| } |
| __declspec(dllexport) uint32_t PM_ETWStartTrace(ETWSessionState* state) { | ||
| ULONG status = | ||
| EnableTraceEx2(state->SessionTraceHandle, (LPCGUID)&DNS_CLIENT_PROVIDER_GUID, | ||
| EVENT_CONTROL_CODE_ENABLE_PROVIDER, | ||
| TRACE_LEVEL_INFORMATION, 0, 0, 0, NULL); | ||
|
|
||
| if (status != ERROR_SUCCESS) { | ||
| return status; | ||
| } | ||
|
|
||
| EVENT_TRACE_LOGFILE trace = { 0 }; | ||
|
|
||
| trace.LoggerName = (LPWSTR)(LOGSESSION_NAME); | ||
| trace.ProcessTraceMode = | ||
| PROCESS_TRACE_MODE_REAL_TIME | PROCESS_TRACE_MODE_EVENT_RECORD; | ||
| trace.EventRecordCallback = EventRecordCallback; | ||
| trace.Context = state; | ||
|
|
||
| state->sessionHandle = OpenTrace(&trace); | ||
| if (state->sessionHandle == INVALID_PROCESSTRACE_HANDLE) { | ||
| return 1; | ||
| } | ||
|
|
||
| status = ProcessTrace(&state->sessionHandle, 1, NULL, NULL); | ||
| if (status != ERROR_SUCCESS) { | ||
| return 1; | ||
| } | ||
|
|
||
| return ERROR_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Add Null Pointer Checks and Improve Error Handling in PM_ETWStartTrace
The function PM_ETWStartTrace does not check if state is NULL before using it. Additionally, error handling can be improved by returning specific error codes.
Apply this diff:
__declspec(dllexport) uint32_t PM_ETWStartTrace(ETWSessionState* state) {
+ if (state == NULL) {
+ return ERROR_INVALID_PARAMETER;
+ }
ULONG status =
EnableTraceEx2(state->SessionTraceHandle, (LPCGUID)&DNS_CLIENT_PROVIDER_GUID,
EVENT_CONTROL_CODE_ENABLE_PROVIDER,
TRACE_LEVEL_INFORMATION, 0, 0, 0, NULL);
if (status != ERROR_SUCCESS) {
return status;
}
// ... [rest of the code remains unchanged]
if (state->sessionHandle == INVALID_PROCESSTRACE_HANDLE) {
- return 1;
+ return ERROR_INVALID_HANDLE;
}
// ... [rest of the code remains unchanged]
if (status != ERROR_SUCCESS) {
- return 1;
+ return status;
}
return ERROR_SUCCESS;
}📝 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.
| __declspec(dllexport) uint32_t PM_ETWStartTrace(ETWSessionState* state) { | |
| ULONG status = | |
| EnableTraceEx2(state->SessionTraceHandle, (LPCGUID)&DNS_CLIENT_PROVIDER_GUID, | |
| EVENT_CONTROL_CODE_ENABLE_PROVIDER, | |
| TRACE_LEVEL_INFORMATION, 0, 0, 0, NULL); | |
| if (status != ERROR_SUCCESS) { | |
| return status; | |
| } | |
| EVENT_TRACE_LOGFILE trace = { 0 }; | |
| trace.LoggerName = (LPWSTR)(LOGSESSION_NAME); | |
| trace.ProcessTraceMode = | |
| PROCESS_TRACE_MODE_REAL_TIME | PROCESS_TRACE_MODE_EVENT_RECORD; | |
| trace.EventRecordCallback = EventRecordCallback; | |
| trace.Context = state; | |
| state->sessionHandle = OpenTrace(&trace); | |
| if (state->sessionHandle == INVALID_PROCESSTRACE_HANDLE) { | |
| return 1; | |
| } | |
| status = ProcessTrace(&state->sessionHandle, 1, NULL, NULL); | |
| if (status != ERROR_SUCCESS) { | |
| return 1; | |
| } | |
| return ERROR_SUCCESS; | |
| } | |
| __declspec(dllexport) uint32_t PM_ETWStartTrace(ETWSessionState* state) { | |
| if (state == NULL) { | |
| return ERROR_INVALID_PARAMETER; | |
| } | |
| ULONG status = | |
| EnableTraceEx2(state->SessionTraceHandle, (LPCGUID)&DNS_CLIENT_PROVIDER_GUID, | |
| EVENT_CONTROL_CODE_ENABLE_PROVIDER, | |
| TRACE_LEVEL_INFORMATION, 0, 0, 0, NULL); | |
| if (status != ERROR_SUCCESS) { | |
| return status; | |
| } | |
| EVENT_TRACE_LOGFILE trace = { 0 }; | |
| trace.LoggerName = (LPWSTR)(LOGSESSION_NAME); | |
| trace.ProcessTraceMode = | |
| PROCESS_TRACE_MODE_REAL_TIME | PROCESS_TRACE_MODE_EVENT_RECORD; | |
| trace.EventRecordCallback = EventRecordCallback; | |
| trace.Context = state; | |
| state->sessionHandle = OpenTrace(&trace); | |
| if (state->sessionHandle == INVALID_PROCESSTRACE_HANDLE) { | |
| return ERROR_INVALID_HANDLE; | |
| } | |
| status = ProcessTrace(&state->sessionHandle, 1, NULL, NULL); | |
| if (status != ERROR_SUCCESS) { | |
| return status; | |
| } | |
| return ERROR_SUCCESS; | |
| } |
| __declspec(dllexport) ETWSessionState* PM_ETWCreateState(GoEventRecordCallback callback) { | ||
| // Create trace session properties. | ||
| ULONG BufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | ||
| EVENT_TRACE_PROPERTIES* SessionProperties = | ||
| (EVENT_TRACE_PROPERTIES*)calloc(1, BufferSize); | ||
| SessionProperties->Wnode.BufferSize = BufferSize; | ||
| SessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | ||
| SessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | ||
| SessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | ||
| SessionProperties->LogFileMode = EVENT_TRACE_REAL_TIME_MODE; | ||
| SessionProperties->MaximumFileSize = 1; // MB | ||
| SessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | ||
|
|
||
| // Create state | ||
| ETWSessionState* state = (ETWSessionState*)calloc(1, sizeof(ETWSessionState)); | ||
| state->SessionProperties = SessionProperties; | ||
| state->callback = callback; | ||
| return state; | ||
| } |
There was a problem hiding this comment.
Add Error Handling for Memory Allocation in PM_ETWCreateState
The function PM_ETWCreateState allocates memory using calloc for SessionProperties and state without checking if the allocation was successful. If memory allocation fails, dereferencing a NULL pointer will cause a crash.
Apply this diff to add error checks:
ULONG BufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME);
EVENT_TRACE_PROPERTIES* SessionProperties =
(EVENT_TRACE_PROPERTIES*)calloc(1, BufferSize);
+if (SessionProperties == NULL) {
+ // Handle memory allocation failure
+ return NULL;
+}
SessionProperties->Wnode.BufferSize = BufferSize;
// ... [rest of the code remains unchanged]
// Create state
ETWSessionState* state = (ETWSessionState*)calloc(1, sizeof(ETWSessionState));
+if (state == NULL) {
+ // Clean up previously allocated memory
+ free(SessionProperties);
+ return NULL;
+}
state->SessionProperties = SessionProperties;
state->callback = callback;
return state;📝 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.
| __declspec(dllexport) ETWSessionState* PM_ETWCreateState(GoEventRecordCallback callback) { | |
| // Create trace session properties. | |
| ULONG BufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | |
| EVENT_TRACE_PROPERTIES* SessionProperties = | |
| (EVENT_TRACE_PROPERTIES*)calloc(1, BufferSize); | |
| SessionProperties->Wnode.BufferSize = BufferSize; | |
| SessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | |
| SessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | |
| SessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | |
| SessionProperties->LogFileMode = EVENT_TRACE_REAL_TIME_MODE; | |
| SessionProperties->MaximumFileSize = 1; // MB | |
| SessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | |
| // Create state | |
| ETWSessionState* state = (ETWSessionState*)calloc(1, sizeof(ETWSessionState)); | |
| state->SessionProperties = SessionProperties; | |
| state->callback = callback; | |
| return state; | |
| } | |
| __declspec(dllexport) ETWSessionState* PM_ETWCreateState(GoEventRecordCallback callback) { | |
| // Create trace session properties. | |
| ULONG BufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | |
| EVENT_TRACE_PROPERTIES* SessionProperties = | |
| (EVENT_TRACE_PROPERTIES*)calloc(1, BufferSize); | |
| if (SessionProperties == NULL) { | |
| // Handle memory allocation failure | |
| return NULL; | |
| } | |
| SessionProperties->Wnode.BufferSize = BufferSize; | |
| SessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | |
| SessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | |
| SessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | |
| SessionProperties->LogFileMode = EVENT_TRACE_REAL_TIME_MODE; | |
| SessionProperties->MaximumFileSize = 1; // MB | |
| SessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | |
| // Create state | |
| ETWSessionState* state = (ETWSessionState*)calloc(1, sizeof(ETWSessionState)); | |
| if (state == NULL) { | |
| // Clean up previously allocated memory | |
| free(SessionProperties); | |
| return NULL; | |
| } | |
| state->SessionProperties = SessionProperties; | |
| state->callback = callback; | |
| return state; | |
| } |
| // PM_ETWStopOldSession removes old session with the same name if they exist. | ||
| // It returns success(0) only if its able to delete the old session. | ||
| __declspec(dllexport) ULONG PM_ETWStopOldSession() { | ||
| ULONG status = ERROR_SUCCESS; | ||
| TRACEHANDLE sessionHandle = 0; | ||
|
|
||
| // Create trace session properties | ||
| size_t bufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | ||
| EVENT_TRACE_PROPERTIES* sessionProperties = (EVENT_TRACE_PROPERTIES*)calloc(1, bufferSize); | ||
| sessionProperties->Wnode.BufferSize = (ULONG)bufferSize; | ||
| sessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | ||
| sessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | ||
| sessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | ||
| sessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | ||
|
|
||
| // Use Control trace will stop the session which will trigger a delete. | ||
| status = ControlTrace(NULL, LOGSESSION_NAME, sessionProperties, EVENT_TRACE_CONTROL_STOP); | ||
|
|
||
| free(sessionProperties); | ||
| return status; | ||
| } |
There was a problem hiding this comment.
Add Error Handling for Memory Allocation in PM_ETWStopOldSession
The function PM_ETWStopOldSession allocates memory using calloc for sessionProperties without checking the result. If the allocation fails, it can lead to a crash when accessing sessionProperties.
Apply this diff:
EVENT_TRACE_PROPERTIES* sessionProperties = (EVENT_TRACE_PROPERTIES*)calloc(1, bufferSize);
+if (sessionProperties == NULL) {
+ // Handle allocation failure
+ return ERROR_OUTOFMEMORY;
+}
sessionProperties->Wnode.BufferSize = (ULONG)bufferSize;
// ... [rest of the code remains unchanged]📝 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.
| // PM_ETWStopOldSession removes old session with the same name if they exist. | |
| // It returns success(0) only if its able to delete the old session. | |
| __declspec(dllexport) ULONG PM_ETWStopOldSession() { | |
| ULONG status = ERROR_SUCCESS; | |
| TRACEHANDLE sessionHandle = 0; | |
| // Create trace session properties | |
| size_t bufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | |
| EVENT_TRACE_PROPERTIES* sessionProperties = (EVENT_TRACE_PROPERTIES*)calloc(1, bufferSize); | |
| sessionProperties->Wnode.BufferSize = (ULONG)bufferSize; | |
| sessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | |
| sessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | |
| sessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | |
| sessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | |
| // Use Control trace will stop the session which will trigger a delete. | |
| status = ControlTrace(NULL, LOGSESSION_NAME, sessionProperties, EVENT_TRACE_CONTROL_STOP); | |
| free(sessionProperties); | |
| return status; | |
| } | |
| // PM_ETWStopOldSession removes old session with the same name if they exist. | |
| // It returns success(0) only if its able to delete the old session. | |
| __declspec(dllexport) ULONG PM_ETWStopOldSession() { | |
| ULONG status = ERROR_SUCCESS; | |
| TRACEHANDLE sessionHandle = 0; | |
| // Create trace session properties | |
| size_t bufferSize = sizeof(EVENT_TRACE_PROPERTIES) + sizeof(LOGSESSION_NAME); | |
| EVENT_TRACE_PROPERTIES* sessionProperties = (EVENT_TRACE_PROPERTIES*)calloc(1, bufferSize); | |
| if (sessionProperties == NULL) { | |
| // Handle allocation failure | |
| return ERROR_OUTOFMEMORY; | |
| } | |
| sessionProperties->Wnode.BufferSize = (ULONG)bufferSize; | |
| sessionProperties->Wnode.Flags = WNODE_FLAG_TRACED_GUID; | |
| sessionProperties->Wnode.ClientContext = 1; // QPC clock resolution | |
| sessionProperties->Wnode.Guid = PORTMASTER_ETW_SESSION_GUID; | |
| sessionProperties->LoggerNameOffset = sizeof(EVENT_TRACE_PROPERTIES); | |
| // Use Control trace will stop the session which will trigger a delete. | |
| status = ControlTrace(NULL, LOGSESSION_NAME, sessionProperties, EVENT_TRACE_CONTROL_STOP); | |
| free(sessionProperties); | |
| return status; | |
| } |
service/firewall/bypassing.go
Outdated
| if conn.Entity.Port == 53 && conn.Entity.IPScope.IsLocalhost() { | ||
| return endpoints.NoMatch, "", nil | ||
| } |
There was a problem hiding this comment.
I think we can allow any normal DNS, as we can see the packets. We only need to prevent encrypted DNS connections from apps.
This is getting complicated. Maybe we can simplify and regroup the logic here? Let's talk about options.
| win32Callback := windows.NewCallback(func(domain *uint16, result *uint16) uintptr { | ||
| callback(windows.UTF16PtrToString(domain), windows.UTF16PtrToString(result)) | ||
| return 0 | ||
| }) |
There was a problem hiding this comment.
Is this true? This seems like a good idea.
(I was referencing something from the bot, but this does not show up here.)
| } | ||
|
|
||
| // Check if the reply indicates the end of the stream | ||
| if flags&varlink.Continues == 0 { |
There was a problem hiding this comment.
If the flag is set, shouldn't the value be > 0, or is this good old 0 == true?
There was a problem hiding this comment.
I don't like the way gofmt formats this.
This checks if the 3-th bit is 1 with an and mask.
varlink.Contunues == 0b00000100
| var ResolverInfo = resolver.ResolverInfo{ | ||
| Name: "SystemdResolver", | ||
| Type: "env", | ||
| Source: "System", | ||
| } |
There was a problem hiding this comment.
We should check how this data is used and introduce new values if needed.
| Name string `json:"name"` | ||
| } | ||
|
|
||
| type ResourceRecord struct { |
There was a problem hiding this comment.
Do we have a source where this came from?
Would be great to link in case something breaks and we need to check if this is still correct.
There was a problem hiding this comment.
The source is:
sudo varlinkctl introspect /run/systemd/resolve/io.systemd.Resolve.Monitor io.systemd.Resolve.Monitor
In varlink the service is required to provide a description of its functions.
I will add a comment
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (32)
service/integration/integration.go (3)
6-6: Add documentation for the OSSpecific type.Consider adding a doc comment explaining the purpose of this type and its relationship with the Windows implementation.
+// OSSpecific provides a platform-specific implementation of OS integration features. +// This is a no-op implementation for non-Windows platforms. type OSSpecific struct{}
8-11: Fix typo and enhance method documentation.The documentation contains a typo and could be more descriptive.
-// Initialize is empty on any OS different then Windows. +// Initialize performs OS-specific initialization. +// This is a no-op implementation for non-Windows platforms. func (i *OSIntegration) Initialize() error {
13-16: Fix typos in CleanUp method documentation.The documentation contains spelling errors.
-// CleanUp releases any resourses allocated during initializaion. +// CleanUp releases any resources allocated during initialization. +// This is a no-op implementation for non-Windows platforms. func (i *OSIntegration) CleanUp() error {service/integration/module.go (1)
45-53: Improve error handling in lifecycle methodsThe Start() and Stop() methods should include proper state management and logging to aid in debugging and ensure clean shutdown.
Consider this enhancement:
func (i *OSIntegration) Start() error { + i.m.StartWorker("os-integration") + defer func() { + if err := recover(); err != nil { + i.m.Error("panic during start: %v", err) + } + }() return i.Initialize() } func (i *OSIntegration) Stop() error { + defer i.m.StopWorker() + defer func() { + if err := recover(); err != nil { + i.m.Error("panic during shutdown: %v", err) + } + }() return i.CleanUp() }service/integration/integration_windows.go (3)
13-16: Add godoc comments for the struct and its fields.The struct is well-designed, but could benefit from documentation explaining its purpose and field usage.
+// OSSpecific holds Windows-specific integration components type OSSpecific struct { + // dll holds the loaded portmaster-core.dll instance dll *windows.DLL + // etwFunctions contains the ETW-related function pointers etwFunctions ETWFunctions }
41-47: Fix typo and improve error handling in CleanUp method
- The comment has typos: "resourses" should be "resources" and "initializaion" should be "initialization"
- Consider wrapping the error with context
-// CleanUp releases any resourses allocated during initializaion. +// CleanUp releases any resources allocated during initialization. func (i *OSIntegration) CleanUp() error { if i.os.dll != nil { - return i.os.dll.Release() + if err := i.os.dll.Release(); err != nil { + return fmt.Errorf("failed to release DLL: %w", err) + } } return nil }
49-52: Fix comment typo and improve documentationThe comment has a typo and could be more descriptive.
-// GetETWInterface return struct containing all the ETW related functions. +// GetETWInterface returns a struct containing all the ETW (Event Tracing for Windows) related functions. func (i *OSIntegration) GetETWInterface() ETWFunctions { return i.os.etwFunctions }service/firewall/interception/dnsmonitor/varlinktypes.go (4)
1-8: Consider enhancing the documentation with protocol version and link.While the source comment is helpful, it would be beneficial to include:
- The protocol version or release date
- A link to the official systemd-resolver documentation
- Examples of typical DNS event structures
9-13: Add field documentation and consider using typed constants.
- Add documentation for each field explaining their purpose and valid values.
- Consider using typed constants for Class and Type fields to prevent invalid values:
+// Common DNS classes +type DNSClass int +const ( + ClassIN DNSClass = 1 // Internet + ClassCS DNSClass = 2 // CSNET + ClassCH DNSClass = 3 // CHAOS + ClassHS DNSClass = 4 // Hesiod +) + +// Common DNS types +type DNSType int +const ( + TypeA DNSType = 1 // IPv4 address + TypeNS DNSType = 2 // Nameserver + TypeCNAME DNSType = 5 // Canonical name + TypeSOA DNSType = 6 // Start of authority + TypePTR DNSType = 12 // Domain name pointer + TypeMX DNSType = 15 // Mail exchange + TypeTXT DNSType = 16 // Text strings + TypeAAAA DNSType = 28 // IPv6 address +) type ResourceKey struct { - Class int `json:"class"` - Type int `json:"type"` + Class DNSClass `json:"class"` + Type DNSType `json:"type"` Name string `json:"name"` }
69-73: Add struct and field documentation.The Answer struct would benefit from documentation explaining:
- The purpose of the struct
- Why Raw is a required field while others are optional
- The relationship between RR and Raw fields
- The significance of IfIndex in DNS responses
Example:
// Answer represents a DNS response containing either a parsed resource record (RR), // the raw response string, or both. IfIndex optionally specifies the network interface // through which the response was received. type Answer struct { RR *ResourceRecord `json:"rr,omitempty"` // Parsed resource record, if available Raw string `json:"raw"` // Raw DNS response string IfIndex *int `json:"ifindex,omitempty"` // Network interface index, if applicable }
75-83: Add documentation and clarify field relationships.
Add comprehensive documentation explaining:
- The purpose of each field
- The relationship between Question and CollectedQuestions
- Valid values for Ready, State, Rcode, and Errno fields
Consider adding constants for common values:
// Query states const ( StateComplete = "complete" StatePending = "pending" StateFailed = "failed" ) // DNS response codes (RCODEs) const ( RcodeNoError = 0 RcodeFormatError = 1 RcodeServerFailure = 2 RcodeNameError = 3 RcodeNotImplemented = 4 RcodeRefused = 5 )service/compat/module.go (3)
102-106: LGTM! Consider standardizing error handling.The early check for disabled resolver is well-placed and prevents unnecessary work. However, for consistency with other skip conditions in this function, consider using the
errSelfcheckSkippederror instead of returning nil.if res.IsDisabled() { log.Debugf("compat: skipping self-check: resolver is disabled") - return nil + return errSelfcheckSkipped }
127-128: Add debug logging for consistency.While the logic is correct, this case lacks debug logging unlike other similar conditions (e.g., network change). This inconsistency could make debugging more difficult.
case res.IsDisabled(): + tracer.Debugf("compat: ignoring self-check failure: resolver is disabled") // Portmaster resolver is disabled, ignore this issue.
Line range hint
41-52: Consider reducing bidirectional package dependencies.The init() function shows that the resolver package depends on compat package functions, and now compat also depends on resolver. This bidirectional dependency could make the system harder to maintain and understand.
Consider introducing an interface layer or moving shared functionality to a separate package to reduce coupling.
service/instance.go (2)
158-161: Maintain consistency in error messages.While the initialization code follows the established pattern, there's a minor inconsistency in the error message format:
- Integration module uses: "create integration module"
- DNS monitor uses: "create dns-listener module"
The module name in the error message should match the struct field name for consistency.
- return instance, fmt.Errorf("create dns-listener module: %w", err) + return instance, fmt.Errorf("create dnsmonitor module: %w", err)Also applies to: 198-201
485-488: Maintain consistency in method comments.For consistency with the struct field name, update the comment:
-// DNSMonitor returns the dns-listener module. +// DNSMonitor returns the dnsmonitor module.service/resolver/resolvers.go (1)
401-408: Consider refactoringloadResolversfor better maintainability.The
loadResolversfunction is quite long and handles multiple responsibilities. Consider breaking it down into smaller, focused functions:
initializeResolverStateloadResolverConfigurationinitializeResolverslogResolverStatusThis would improve:
- Code maintainability
- Testing capabilities
- Error handling granularity
Example refactor for the state management:
+func initializeResolverState(config []string) { + if len(config) == 0 { + module.isDisabled.Store(true) + } else { + module.isDisabled.Store(false) + } +} + func loadResolvers() { defer func() { if !allConfiguredResolversAreFailing() { resetFailingResolversNotification() } }() resolversLock.Lock() defer resolversLock.Unlock() module.states.Remove(missingResolversErrorID) newResolverConfig := configuredNameServers() if len(currentResolverConfig) > 0 && !utils.StringSliceEqual(currentResolverConfig, newResolverConfig) { module.mgr.Go("clear dns cache", func(ctx *mgr.WorkerCtx) error { log.Info("resolver: clearing dns cache due to changed resolver config") _, err := clearNameCache(ctx.Ctx()) return err }) } - if len(newResolverConfig) == 0 { - module.isDisabled.Store(true) - } else { - module.isDisabled.Store(false) - } + initializeResolverState(newResolverConfig)Also applies to: 441-441
service/firewall/interception/dnsmonitor/eventlistener_windows.go (5)
23-23: Correct typographical error in comment.On line 23, the comment contains a typo: "Intialize" should be "Initialize."
Apply this diff to fix the typo:
- // Intialize new dns event session. + // Initialize new DNS event session.
29-29: Correct typographical error in comment.On line 29, the comment contains a typo: "lisening" should be "listening."
Apply this diff to fix the typo:
- // Start lisening for events. + // Start listening for events.
48-48: Correct typographical errors in comment.On line 48, the comment contains typos: "Destory" should be "Destroy," and "failes" should be "fails."
Apply this diff to correct the typos:
- // Stop and destroy trace. Destory should be called even if stop failes for some reason. + // Stop and destroy trace. Destroy should be called even if stop fails for some reason.
57-57: Fix typographical error in error message.On line 57, the error message contains a typo: "DestorySession" should be "DestroySession."
Apply this diff to fix the typo:
- return fmt.Errorf("DestorySession failed: %w", err2) + return fmt.Errorf("DestroySession failed: %w", err2)
49-59: Consider combining errors fromStopTraceandDestroySession.Currently, if both
StopTraceandDestroySessionfail, only one error is returned, and the other is discarded. To provide comprehensive error information, consider combining both errors.Here's a possible way to handle both errors:
err := l.etw.StopTrace() err2 := l.etw.DestroySession() if err != nil || err2 != nil { var combinedError string if err != nil { combinedError += fmt.Sprintf("StopTrace failed: %v. ", err) } if err2 != nil { combinedError += fmt.Sprintf("DestroySession failed: %v.", err2) } return fmt.Errorf(combinedError) } return nilservice/firewall/interception/dnsmonitor/eventlistener_linux.go (5)
21-21: Consider making Varlink service address configurableCurrently, the Varlink connection is established using a hardcoded address
"unix:/run/systemd/resolve/io.systemd.Resolve.Monitor". To improve flexibility and adaptability across different environments, consider making this address configurable or deriving it from a constant.
32-38: Include method name in error messages for clarityWhen handling errors from the Varlink call, including the method name
"io.systemd.Resolve.Monitor.SubscribeQueryResults"in the error messages can provide more context and assist in debugging.
58-60: Log ignored DNS errors for monitoringCurrently, DNS errors are ignored when
queryResult.Rcode != nil. Consider logging these errors to monitor DNS issues, which can be valuable for troubleshooting and performance analysis.
82-82: Correct the comment grammarThe comment should read "Allocate data structures for the parsed result." instead of "Allocated data struct for the parsed result."
93-93: Fix typo in comment: "trough" to "through"There's a typo in the comment. Replace "Go trough each answer entry." with "Go through each answer entry."
service/network/connection.go (1)
552-561: Log a message if DNS retries failIf all retries to obtain DNS information fail, currently no message is logged. Adding a log message can aid in debugging and monitoring potential issues with DNS resolution.
Apply this diff to add a log message after all retries have been exhausted:
} time.Sleep(5 * time.Millisecond) } + if err != nil { + log.Tracer(pkt.Ctx()).Warningf("network: failed to retrieve domain after %d retries", i) + } }service/firewall/interception/dnsmonitor/etwlink_windows.go (4)
25-54: Correct spelling mistakes in comments and error messagesThere are typographical errors in the
NewSessionfunction:
- Line 25~: "initilizes" should be "initializes", and "DestorySession" should be "DestroySession".
- Line 50~: "initialzie" should be "initialize".
Correcting these typos will enhance code readability and professionalism.
79-79: Fix typographical error in method commentIn the comment for
StopTrace(line 79~), "stopes" should be "stops".
43-45: Avoid relying on finalizers for critical resource cleanupUsing
runtime.SetFinalizerto callDestroySessionmay not guarantee timely release of resources, as finalizers run unpredictably during garbage collection. This can lead to resource leaks or delayed cleanup.It's recommended to ensure that
DestroySessionis explicitly called when the session is no longer needed. Consider removing the finalizer or clarifying in the documentation the importance of callingDestroySessionpromptly.
25-99: Follow GoDoc conventions for method commentsFor improved documentation and consistency, method comments should start with the method name. For example:
- Change (line 25~):
To:// NewSession creates new ETW event listener and initilizes it.// NewSession creates a new ETW event listener and initializes it.Apply this pattern to all method comments to adhere to GoDoc standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
service/compat/module.go(3 hunks)service/firewall/bypassing.go(1 hunks)service/firewall/interception/dnsmonitor/etwlink_windows.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_linux.go(1 hunks)service/firewall/interception/dnsmonitor/eventlistener_windows.go(1 hunks)service/firewall/interception/dnsmonitor/module.go(1 hunks)service/firewall/interception/dnsmonitor/varlinktypes.go(1 hunks)service/firewall/packet_handler.go(1 hunks)service/instance.go(9 hunks)service/integration/etw_windows.go(1 hunks)service/integration/integration.go(1 hunks)service/integration/integration_windows.go(1 hunks)service/integration/module.go(1 hunks)service/network/connection.go(4 hunks)service/network/module.go(2 hunks)service/resolver/main.go(2 hunks)service/resolver/resolvers.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- service/firewall/interception/dnsmonitor/eventlistener.go
🚧 Files skipped from review as they are similar to previous changes (4)
- service/firewall/bypassing.go
- service/firewall/packet_handler.go
- service/integration/etw_windows.go
- service/resolver/main.go
🧰 Additional context used
🪛 golangci-lint
service/instance.go
395-395: intergration is a misspelling of integration
(misspell)
🔇 Additional comments (14)
service/integration/integration.go (1)
1-2: LGTM! Build constraints follow Go's compatibility guidelines.
The dual build constraints properly handle both new and legacy Go versions.
service/integration/module.go (2)
11-20: Previous review comment about nil pointer is still valid
The os field initialization issue mentioned in the previous review remains unaddressed. This could lead to runtime panics if the field is accessed before being properly set.
46-53: Verify platform-specific implementations
The Start() and Stop() methods delegate to Initialize() and CleanUp(), which are platform-specific. Let's verify their implementations.
✅ Verification successful
Based on the search results, I can see that the code has proper cleanup handling for resources:
- The
Initialize()andCleanUp()methods are implemented in both Windows and non-Windows platforms:
- Windows (
integration_windows.go): Properly releases DLL resources inCleanUp() - Non-Windows (
integration.go): Has empty implementations since no resources need cleanup
-
The
Start()andStop()methods inmodule.gocorrectly delegate toInitialize()andCleanUp()respectively. -
The Windows implementation properly handles DLL loading and unloading:
- Loads DLL in
Initialize() - Releases DLL in
CleanUp()when it exists
- The error handling is appropriate, with errors being propagated up the call chain.
The implementation follows proper resource management practices, with resources being initialized and cleaned up in a symmetric way. The Windows-specific code properly manages native resources through the DLL lifecycle.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check platform-specific implementations of Initialize and CleanUp
echo "Checking Initialize() implementations:"
rg -A 5 "Initialize\(\)" --type go
echo "Checking CleanUp() implementations:"
rg -A 5 "CleanUp\(\)" --type go
echo "Checking for potential resource handling:"
rg -A 5 "(Open|Close|Load|Unload)" --type go
Length of output: 272295
service/integration/integration_windows.go (2)
1-11: LGTM! Build constraints and imports are properly configured.
The file correctly uses both old and new style build constraints for Windows, and imports the necessary packages.
19-39:
Add cleanup handling for initialization failures
The initialization process needs proper cleanup in case of failures after the DLL is loaded. This prevents resource leaks.
Apply this fix as suggested in the previous review:
func (i *OSIntegration) Initialize() error {
// Find path to the dll.
file, err := updates.GetFile("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)
}
+ // Ensure cleanup on failure
+ var success bool
+ defer func() {
+ if !success {
+ 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
}
+ success = true
return nil
}service/firewall/interception/dnsmonitor/varlinktypes.go (1)
1-83: Verify protocol compatibility and completeness.
Let's verify that this implementation covers all required DNS record types and matches the systemd-resolver protocol specification.
✅ Verification successful
Let me analyze the DNS record types and protocol compatibility based on the search results.
The code in varlinktypes.go defines structs for systemd-resolver varlink DNS event protocol, but we need to verify if it covers all required DNS record types and matches the protocol specification. Let me search for more specific information about DNS record type handling.
Based on the search results, I can now provide a final verification response:
Implementation matches protocol specification but has limited record type support
The implementation correctly follows the systemd-resolver varlink DNS event protocol as documented in the source comment (sudo varlinkctl introspect /run/systemd/resolve/io.systemd.Resolve.Monitor). However:
-
The code currently only actively uses a subset of DNS record types:
- A and AAAA records for IPv4/IPv6 addresses
- CNAME records for aliases
- PTR records for reverse lookups
- SOA records for zone information
- SVCB and HTTPS records for service binding
- OPT and NULL records for protocol extensions
-
The
ResourceRecordstruct includes commented fields for many other record types (MX, NS, TXT, etc.) but they are marked as "not used"
This implementation is sufficient for basic DNS resolution but may need to uncomment and implement handling for additional record types if more comprehensive DNS record support is required in the future.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify DNS record types coverage and protocol compatibility
# Check if we have all common DNS record types
echo "Checking for common DNS record types in the code..."
rg "Type[A-Z]+" -A 1
# Look for systemd-resolver protocol documentation or tests
echo "Looking for systemd-resolver protocol documentation..."
fd -g "*resolve*.varlink" -x cat {} \;
fd -g "*dns*.varlink" -x cat {} \;
# Search for related test files
echo "Looking for related test files..."
fd -g "*dns*test*.go" -x cat {} \;
fd -g "*resolve*test*.go" -x cat {} \;
Length of output: 92507
Script:
#!/bin/bash
# Search for DNS record type handling in the codebase
# Look for DNS record type constants and usage
echo "Checking DNS record type handling..."
rg "dns\.Type[A-Z]+" -A 1
# Look for systemd-resolver protocol documentation
echo "Looking for systemd-resolver protocol documentation..."
rg -i "systemd.*resolver.*protocol" -A 3
rg -i "varlink.*protocol" -A 3
# Look for any additional DNS record type handling
echo "Looking for additional DNS record type handling..."
rg "ResourceRecord" -A 5
Length of output: 5723
service/network/module.go (2)
12-12: LGTM: Import statements are correctly added
The new imports for dnsmonitor and resolver packages are properly placed and align with the interface enhancements.
Also applies to: 17-17
193-194: Verify implementations of new interface methods
The interface additions look good, but all implementers of the instance interface must be updated to include these methods.
Let's verify the implementations:
✅ Verification successful
Interface methods are properly implemented
The verification shows that all new interface methods are correctly implemented in the main Instance struct in service/instance.go:
Profile()implementation existsResolver()implementation existsDNSMonitor()implementation exists
No other implementers of the interface were found in the codebase, indicating that Instance is the only implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all implementers of the instance interface and verify method implementations
# Find all types that implement Profile() method (existing interface method)
echo "=== Searching for instance interface implementers ==="
ast-grep --pattern 'func ($receiver *$name) Profile() *profile.ProfileModule {
$$$
}'
# Verify new method implementations
echo -e "\n=== Checking Resolver() implementation ==="
ast-grep --pattern 'func ($receiver *$name) Resolver() *resolver.ResolverModule {
$$$
}'
echo -e "\n=== Checking DNSMonitor() implementation ==="
ast-grep --pattern 'func ($receiver *$name) DNSMonitor() *dnsmonitor.DNSMonitor {
$$$
}'
# Check test implementations
echo -e "\n=== Checking test implementations ==="
fd -e go -x grep -l "type.*struct.*testing.T"
Length of output: 737
Script:
#!/bin/bash
# Let's try a different approach to find implementers and their methods
# First, find all Go files that might contain implementations
echo "=== Finding relevant Go files ==="
rg -l "type.*?struct" --type go
# Then search for Profile method to identify implementers
echo -e "\n=== Finding Profile() implementations ==="
rg "func \(.*?\) Profile\(\) \*profile\.ProfileModule" -A 1
# Search for new method implementations
echo -e "\n=== Finding Resolver() implementations ==="
rg "func \(.*?\) Resolver\(\) \*resolver\.ResolverModule" -A 1
echo -e "\n=== Finding DNSMonitor() implementations ==="
rg "func \(.*?\) DNSMonitor\(\) \*dnsmonitor\.DNSMonitor" -A 1
# Look for mock implementations in test files
echo -e "\n=== Finding mock implementations in tests ==="
rg "type.*?Mock.*?struct" --type go
Length of output: 12673
service/instance.go (3)
22-23: LGTM: Import statements are properly organized.
The new imports are correctly placed and follow the existing grouping pattern.
70-70: LGTM: Fields are properly organized within the Instance struct.
The new fields are correctly placed near related modules, maintaining the logical grouping of the struct fields.
Also applies to: 80-80
290-290: LGTM: Modules are properly registered in the service group.
The new modules are correctly placed near related modules, maintaining the logical grouping in the service initialization.
Also applies to: 304-304
service/resolver/resolvers.go (2)
441-441: LGTM! Fixed typo in comment.
The comment text has been corrected from "assing" to "assign".
401-408: Verify the impact of resolver disabled state on dependent modules.
The new code correctly manages the resolver's disabled state, but we should verify how other modules handle this state change.
✅ Verification successful
Based on the search results, I can now generate a final response:
The resolver's disabled state management is safely implemented and properly consumed
The disabled state is:
- Correctly managed using atomic operations in
service/resolver/resolvers.go - Only checked via the exported
IsDisabled()method inservice/resolver/main.go - Used by the UI layer to reflect system state in
desktop/angular/src/app/shared/config
The implementation is thread-safe and follows a clear pattern where state changes are properly propagated to dependent modules through the UI layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find modules that check the resolver's disabled state
# to ensure they handle the state change appropriately.
# Search for references to the disabled state
rg -A 5 "isDisabled"
# Search for direct resolver state checks
ast-grep --pattern 'if $resolver.$state {
$$$
}'
Length of output: 5188
service/firewall/interception/dnsmonitor/module.go (1)
77-85:
Correct loop syntax in saveDomain function
The loop for range 50 is not correctly iterating 50 times. To loop a fixed number of times, use a counter-controlled loop.
Apply this diff to fix the loop:
- for range 50 {
+ for i := 0; i < 50; i++ {
nextDomain, isCNAME := cnames[domain]
if !isCNAME || nextDomain == domain {
break
}
record.CNAMEs = append(record.CNAMEs, nextDomain)
domain = nextDomain
}Likely invalid or redundant comment.
| //go:build !windows | ||
| // +build !windows | ||
|
|
||
| package integration | ||
|
|
||
| type OSSpecific struct{} | ||
|
|
||
| // Initialize is empty on any OS different then Windows. | ||
| func (i *OSIntegration) Initialize() error { | ||
| return nil | ||
| } | ||
|
|
||
| // CleanUp releases any resourses allocated during initializaion. | ||
| func (i *OSIntegration) CleanUp() error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for platform-specific implementations
The codebase has proper platform-specific implementations with build tags (integration.go for non-Windows and integration_windows.go for Windows), but lacks test files for either platform. This is concerning as both the no-op and Windows-specific implementations should be tested.
- No test files found in
service/integration/directory - Platform-specific implementations exist:
- Non-Windows:
integration.go(//go:build !windows) - Windows:
integration_windows.go(//go:build windows) - Windows ETW:
etw_windows.go(//go:build windows)
- Non-Windows:
🔗 Analysis chain
Verify cross-platform testing coverage.
The no-op implementation looks correct, but ensure it's properly tested on non-Windows platforms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing tests of this implementation
rg -g "!vendor/*" -g "*_test.go" "TestOSIntegration.*Initialize|TestOSIntegration.*CleanUp"
# Check if there are any platform-specific test files
fd -g "integration_test*" --type f
Length of output: 130
Script:
#!/bin/bash
# Let's try a broader search for test files and platform-specific files
fd -e go --type f . service/integration/
# Check for any test functions related to OSIntegration
rg -g "*.go" "func.*Test.*OSIntegration" service/integration/
# Look for build tags to understand test coverage
rg -g "*.go" "^//\s*\+build|^//go:build" service/integration/
Length of output: 652
service/integration/module.go
Outdated
| var shimLoaded atomic.Bool | ||
|
|
||
| // New returns a new OSIntegration module. | ||
| func New(instance instance) (*OSIntegration, error) { | ||
| if !shimLoaded.CompareAndSwap(false, true) { | ||
| return nil, errors.New("only one instance allowed") | ||
| } | ||
|
|
||
| m := mgr.New("OSIntegration") | ||
| module := &OSIntegration{ | ||
| m: m, | ||
| states: m.NewStateMgr(), | ||
|
|
||
| instance: instance, | ||
| } | ||
|
|
||
| return module, nil | ||
| } |
There was a problem hiding this comment.
Add cleanup mechanism for singleton state
The singleton state (shimLoaded) is not reset if initialization fails or when the module is stopped. This could prevent creating a new instance after a failed attempt or module shutdown.
Consider this implementation:
func New(instance instance) (*OSIntegration, error) {
if !shimLoaded.CompareAndSwap(false, true) {
return nil, errors.New("only one instance allowed")
}
+
+ // Reset shimLoaded if initialization fails
+ defer func() {
+ if recover() != nil {
+ shimLoaded.Store(false)
+ }
+ }()
m := mgr.New("OSIntegration")
module := &OSIntegration{Also add a reset in the Stop() method:
func (i *OSIntegration) Stop() error {
- return i.CleanUp()
+ err := i.CleanUp()
+ shimLoaded.Store(false)
+ return err
}Committable suggestion skipped: line range outside the PR's diff.
| type ResourceRecord struct { | ||
| Key ResourceKey `json:"key"` | ||
| Name *string `json:"name,omitempty"` | ||
| Address *[]byte `json:"address,omitempty"` | ||
| // Rest of the fields are not used. | ||
| // Priority *int `json:"priority,omitempty"` | ||
| // Weight *int `json:"weight,omitempty"` | ||
| // Port *int `json:"port,omitempty"` | ||
| // CPU *string `json:"cpu,omitempty"` | ||
| // OS *string `json:"os,omitempty"` | ||
| // Items *[]string `json:"items,omitempty"` | ||
| // MName *string `json:"mname,omitempty"` | ||
| // RName *string `json:"rname,omitempty"` | ||
| // Serial *int `json:"serial,omitempty"` | ||
| // Refresh *int `json:"refresh,omitempty"` | ||
| // Expire *int `json:"expire,omitempty"` | ||
| // Minimum *int `json:"minimum,omitempty"` | ||
| // Exchange *string `json:"exchange,omitempty"` | ||
| // Version *int `json:"version,omitempty"` | ||
| // Size *int `json:"size,omitempty"` | ||
| // HorizPre *int `json:"horiz_pre,omitempty"` | ||
| // VertPre *int `json:"vert_pre,omitempty"` | ||
| // Latitude *int `json:"latitude,omitempty"` | ||
| // Longitude *int `json:"longitude,omitempty"` | ||
| // Altitude *int `json:"altitude,omitempty"` | ||
| // KeyTag *int `json:"key_tag,omitempty"` | ||
| // Algorithm *int `json:"algorithm,omitempty"` | ||
| // DigestType *int `json:"digest_type,omitempty"` | ||
| // Digest *string `json:"digest,omitempty"` | ||
| // FPType *int `json:"fptype,omitempty"` | ||
| // Fingerprint *string `json:"fingerprint,omitempty"` | ||
| // Flags *int `json:"flags,omitempty"` | ||
| // Protocol *int `json:"protocol,omitempty"` | ||
| // DNSKey *string `json:"dnskey,omitempty"` | ||
| // Signer *string `json:"signer,omitempty"` | ||
| // TypeCovered *int `json:"type_covered,omitempty"` | ||
| // Labels *int `json:"labels,omitempty"` | ||
| // OriginalTTL *int `json:"original_ttl,omitempty"` | ||
| // Expiration *int `json:"expiration,omitempty"` | ||
| // Inception *int `json:"inception,omitempty"` | ||
| // Signature *string `json:"signature,omitempty"` | ||
| // NextDomain *string `json:"next_domain,omitempty"` | ||
| // Types *[]int `json:"types,omitempty"` | ||
| // Iterations *int `json:"iterations,omitempty"` | ||
| // Salt *string `json:"salt,omitempty"` | ||
| // Hash *string `json:"hash,omitempty"` | ||
| // CertUsage *int `json:"cert_usage,omitempty"` | ||
| // Selector *int `json:"selector,omitempty"` | ||
| // MatchingType *int `json:"matching_type,omitempty"` | ||
| // Data *string `json:"data,omitempty"` | ||
| // Tag *string `json:"tag,omitempty"` | ||
| // Value *string `json:"value,omitempty"` | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve handling of unused fields.
The current approach of commenting out unused fields has several drawbacks:
- It clutters the code
- It's unclear if these fields will be needed in the future
- The maintenance burden of keeping commented code in sync
Consider either:
- Removing the commented fields and tracking them in documentation/issue tracker
- Or, if they're planned for future use, add a more descriptive comment explaining when and why they'll be needed
| @@ -181,4 +189,5 @@ func New(instance instance) (*Compat, error) { | |||
|
|
|||
| type instance interface { | |||
| NetEnv() *netenv.NetEnv | |||
| Resolver() *resolver.ResolverModule | |||
There was a problem hiding this comment.
💡 Codebase verification
Missing Resolver() implementation in spn/instance.go
The verification reveals that spn/instance.go implements NetEnv() but lacks the newly added Resolver() method. This implementation needs to be updated to maintain interface compatibility.
spn/instance.go: Add the missingResolver()method to match the interface requirements- Other implementations in
service/instance.goand test files are correctly updated
🔗 Analysis chain
Verify interface implementation across the codebase.
The addition of Resolver() method to the interface looks good, but we should verify that all types implementing this interface have been updated.
The previous attempt might have failed due to complex AST pattern. Let's try using ripgrep to:
- Find the interface definition to confirm the methods we need to check
- Search for implementations of key methods (NetEnv and Resolver) to identify structs that implement them
The previous output shows we have potentially found an issue - the spn/instance.go implementation has NetEnv() but might be missing Resolver(). Let's verify the complete interface definition and check the full implementation in spn/instance.go to confirm this.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for types that implement the instance interface
ast-grep --pattern 'type $_ struct {
$$$
}
func ($_ $_) NetEnv() *netenv.NetEnv {
$$$
}'
# This will help identify structs that implement NetEnv() but might be missing Resolver()
Length of output: 95
Script:
#!/bin/bash
# Let's try a different approach to find interface implementations
# First, let's find the interface definition
echo "=== Interface Definition ==="
rg -A 5 "type Instance interface"
# Then search for struct types that might implement it
echo -e "\n=== Potential Implementations ==="
rg "func \(.*?\) (NetEnv|Resolver)\(\)" --type go -A 1
Length of output: 695
Script:
#!/bin/bash
# Let's find the complete interface definition first
echo "=== Complete Interface Definition ==="
rg -B 2 -A 10 "type Instance interface"
# Let's also check the implementation in spn/instance.go since it's missing Resolver()
echo -e "\n=== SPN Instance Implementation ==="
rg -B 2 -A 15 "type Instance struct" spn/instance.go
Length of output: 649
| cnames := make(map[string]string) | ||
| ips := []net.IP{} | ||
|
|
||
| resultArray := strings.Split(result, ";") | ||
| for _, r := range resultArray { | ||
| // For results other than IP addresses, the string starts with "type:" | ||
| if strings.HasPrefix(r, "type:") { | ||
| dnsValueArray := strings.Split(r, " ") | ||
| if len(dnsValueArray) < 3 { | ||
| continue | ||
| } | ||
|
|
||
| // Ignore everything except CNAME records | ||
| if value, err := strconv.ParseInt(dnsValueArray[1], 10, 16); err == nil && value == int64(dns.TypeCNAME) { | ||
| cnames[domain] = dnsValueArray[2] | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle multiple CNAME records per domain.
Currently, the cnames map stores only one CNAME per domain, which may overwrite previous entries if multiple CNAME records exist. To accurately capture all CNAME records, consider changing cnames to map to a slice of strings.
Apply this diff to adjust the cnames map and store all CNAMEs:
- cnames := make(map[string]string)
+ cnames := make(map[string][]string)
...
- cnames[domain] = dnsValueArray[2]
+ cnames[domain] = append(cnames[domain], dnsValueArray[2])📝 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.
| cnames := make(map[string]string) | |
| ips := []net.IP{} | |
| resultArray := strings.Split(result, ";") | |
| for _, r := range resultArray { | |
| // For results other than IP addresses, the string starts with "type:" | |
| if strings.HasPrefix(r, "type:") { | |
| dnsValueArray := strings.Split(r, " ") | |
| if len(dnsValueArray) < 3 { | |
| continue | |
| } | |
| // Ignore everything except CNAME records | |
| if value, err := strconv.ParseInt(dnsValueArray[1], 10, 16); err == nil && value == int64(dns.TypeCNAME) { | |
| cnames[domain] = dnsValueArray[2] | |
| } | |
| cnames := make(map[string][]string) | |
| ips := []net.IP{} | |
| resultArray := strings.Split(result, ";") | |
| for _, r := range resultArray { | |
| // For results other than IP addresses, the string starts with "type:" | |
| if strings.HasPrefix(r, "type:") { | |
| dnsValueArray := strings.Split(r, " ") | |
| if len(dnsValueArray) < 3 { | |
| continue | |
| } | |
| // Ignore everything except CNAME records | |
| if value, err := strconv.ParseInt(dnsValueArray[1], 10, 16); err == nil && value == int64(dns.TypeCNAME) { | |
| cnames[domain] = append(cnames[domain], dnsValueArray[2]) | |
| } |
| dl.listener, err = newListener(dl) | ||
| if err != nil { | ||
| log.Errorf("failed to start dns listener: %s", err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Return error when listener initialization fails in Start() method
The Start() method should return the error encountered during listener initialization instead of always returning nil. This ensures that any issues during startup are propagated to the caller for proper handling.
Apply this diff to return the error:
func (dl *DNSMonitor) Start() error {
// Initialize dns event listener
var err error
dl.listener, err = newListener(dl)
if err != nil {
log.Errorf("failed to start dns listener: %s", err)
+ return 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.
| dl.listener, err = newListener(dl) | |
| if err != nil { | |
| log.Errorf("failed to start dns listener: %s", err) | |
| } | |
| return nil | |
| } | |
| dl.listener, err = newListener(dl) | |
| if err != nil { | |
| log.Errorf("failed to start dns listener: %s", err) | |
| return err | |
| } | |
| } |
| return dl.listener.flush() | ||
| } | ||
|
|
There was a problem hiding this comment.
Check for nil listener before flushing in Flush() method
The Flush() method should check if dl.listener is not nil before calling flush(), to prevent a potential nil pointer dereference.
Apply this diff to add a nil check:
func (dl *DNSMonitor) Flush() error {
+ if dl.listener != nil {
return dl.listener.flush()
+ }
+ 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.
| return dl.listener.flush() | |
| } | |
| func (dl *DNSMonitor) Flush() error { | |
| if dl.listener != nil { | |
| return dl.listener.flush() | |
| } | |
| return nil | |
| } |
| err := dl.listener.stop() | ||
| if err != nil { | ||
| log.Errorf("failed to close listener: %s", err) | ||
| } | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Return error when listener shutdown fails in Stop() method
Similarly, the Stop() method should return any error encountered when stopping the listener, instead of always returning nil.
Apply this diff to return the error:
func (dl *DNSMonitor) Stop() error {
if dl.listener != nil {
err := dl.listener.stop()
if err != nil {
log.Errorf("failed to close listener: %s", err)
+ return 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.
| err := dl.listener.stop() | |
| if err != nil { | |
| log.Errorf("failed to close listener: %s", err) | |
| } | |
| } | |
| return nil | |
| } | |
| err := dl.listener.stop() | |
| if err != nil { | |
| log.Errorf("failed to close listener: %s", err) | |
| return err | |
| } | |
| } | |
| } |
| if module.instance.Resolver().IsDisabled() && conn.shouldWaitForDomain() { | ||
| // Flush the dns listener buffer and try again. | ||
| for i := range 4 { | ||
| _ = module.instance.DNSMonitor().Flush() |
There was a problem hiding this comment.
Handle the error returned by Flush
The error returned by module.instance.DNSMonitor().Flush() is currently ignored. If Flush() fails, it might affect subsequent DNS information retrieval. Consider handling the error appropriately to ensure reliability.
Apply this diff to handle the error:
- _ = module.instance.DNSMonitor().Flush()
+ if errFlush := module.instance.DNSMonitor().Flush(); errFlush != nil {
+ log.Tracer(pkt.Ctx()).Warningf("Failed to flush DNS monitor: %v", errFlush)
+ // Optionally, handle the error as needed
+ }Committable suggestion skipped: line range outside the PR's diff.
| func (l *ETWSession) StartTrace() error { | ||
| return l.i.StartTrace(l.state) | ||
| } |
There was a problem hiding this comment.
Prevent method calls after session is destroyed
The methods StartTrace (lines 57-59~) and StopTrace (lines 81-82~) do not check if the session has been destroyed before proceeding. Calling these methods after DestroySession could result in undefined behavior or errors due to the invalid l.state.
Add checks to ensure the session is still running before executing these methods.
Apply the following changes to add safety checks:
func (l *ETWSession) StartTrace() error {
+ if l.shutdownGuard.Load() {
+ return fmt.Errorf("cannot start trace; session has been destroyed")
+ }
return l.i.StartTrace(l.state)
}
func (l *ETWSession) StopTrace() error {
+ if l.shutdownGuard.Load() {
+ return fmt.Errorf("cannot stop trace; session has been destroyed")
+ }
return l.i.StopTrace(l.state)
}Also applies to: 81-82
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
service/integration/module.go(1 hunks)
🧰 Additional context used
🪛 golangci-lint
service/integration/module.go
13-13: field os is unused
(unused)
🔇 Additional comments (3)
service/integration/module.go (3)
1-6: Clean package structure and imports
36-44: Improve lifecycle methods with proper error handling and documentation
The Start() and Stop() methods have several concerns:
- They depend on undefined methods (Initialize and CleanUp)
- No error handling or logging
- Potential resource leaks in Stop() as mentioned in previous reviews
Please:
- Document where Initialize() and CleanUp() are implemented
- Add error handling and logging
- Ensure proper resource cleanup in Stop()
46-48: Clean interface definition
The interface follows good design practices with a single, focused responsibility.
| type OSIntegration struct { | ||
| m *mgr.Manager | ||
| states *mgr.StateMgr | ||
|
|
||
| os OSSpecific | ||
|
|
||
| instance instance | ||
| } |
There was a problem hiding this comment.
Initialize and document the os field
The os field of type OSSpecific requires attention:
- It's currently uninitialized, which could lead to nil pointer dereferences
- Static analysis indicates it's unused in the current implementation
Consider one of these approaches:
- Initialize it in the constructor
- Add documentation explaining when and where this field should be initialized
- If it's for future use, add a TODO comment explaining its intended purpose
🧰 Tools
🪛 golangci-lint
13-13: field os is unused
(unused)
| // New returns a new OSIntegration module. | ||
| func New(instance instance) (*OSIntegration, error) { | ||
| m := mgr.New("OSIntegration") | ||
| module := &OSIntegration{ | ||
| m: m, | ||
| states: m.NewStateMgr(), | ||
|
|
||
| instance: instance, | ||
| } | ||
|
|
||
| return module, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance constructor with validation and proper initialization
The constructor needs improvements in several areas:
- Add input validation for the
instanceparameter - Initialize the
osfield or document why it's left uninitialized - Consider adding error handling for potential initialization failures
Consider this implementation:
func New(instance instance) (*OSIntegration, error) {
+ if instance == nil {
+ return nil, errors.New("instance cannot be nil")
+ }
+
m := mgr.New("OSIntegration")
module := &OSIntegration{
m: m,
states: m.NewStateMgr(),
instance: instance,
+ os: OSSpecific{}, // Initialize if needed
}
return module, nil
}Committable suggestion skipped: line range outside the PR's diff.
Copilot
AI
left a comment
Copilot
AI
left a comment
There was a problem hiding this comment.
Copilot reviewed 16 out of 31 changed files in this pull request and generated 3 suggestions.
Files not reviewed (15)
- go.mod: Language not supported
- service/network/module.go: Evaluated as low risk
- service/network/connection.go: Evaluated as low risk
- service/firewall/packet_handler.go: Evaluated as low risk
- service/resolver/main.go: Evaluated as low risk
- service/firewall/module.go: Evaluated as low risk
- service/firewall/bypassing.go: Evaluated as low risk
- service/instance.go: Evaluated as low risk
- service/compat/module.go: Evaluated as low risk
- service/resolver/resolvers.go: Evaluated as low risk
- service/firewall/interception/dnsmonitor/varlinktypes.go: Evaluated as low risk
- service/firewall/interception/dnsmonitor/eventlistener.go: Evaluated as low risk
- service/integration/integration.go: Evaluated as low risk
- service/integration/etw_windows.go: Evaluated as low risk
- service/integration/integration_windows.go: Evaluated as low risk
Comments skipped due to low confidence (2)
service/firewall/interception/dnsmonitor/eventlistener_windows.go:43
- [nitpick] The error message 'listener is nil' is not very descriptive. Consider changing it to 'Listener instance is nil'.
return fmt.Errorf("listener is nil")
service/firewall/interception/dnsmonitor/eventlistener_windows.go:46
- [nitpick] The error message 'invalid etw session' is not very descriptive. Consider changing it to 'ETW session instance is invalid'.
return fmt.Errorf("invalid etw session")
|
|
||
| func newListener(module *DNSMonitor) (*Listener, error) { | ||
| // Create the varlink connection with the systemd resolver. | ||
| varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor") |
There was a problem hiding this comment.
The context should be passed from the module itself, not from the manager. Use module.Ctx() instead of module.mgr.Ctx().
| varlinkConn, err := varlink.NewConnection(module.mgr.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor") | |
| varlinkConn, err := varlink.NewConnection(module.Ctx(), "unix:/run/systemd/resolve/io.systemd.Resolve.Monitor") |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| func newListener(module *DNSMonitor) (*Listener, error) { | ||
| listener := &Listener{} | ||
| var err error | ||
| // Intialize new dns event session. |
There was a problem hiding this comment.
The word 'Intialize' is misspelled. It should be 'Initialize'.
| // Intialize new dns event session. | |
| // Initialize new dns event session. |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| if l.etw == nil { | ||
| return fmt.Errorf("invalid etw session") | ||
| } | ||
| // Stop and destroy trace. Destory should be called even if stop failes for some reason. |
There was a problem hiding this comment.
The word 'Destory' is misspelled. It should be 'Destroy'.
| // Stop and destroy trace. Destory should be called even if stop failes for some reason. | |
| // Stop and destroy trace. Destroy should be called even if stop failes for some reason. |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores