Skip to content

Commit da6b66e

Browse files
authored
fix: nil pointer panic in AWS ALB provider crashes process (#743)
* fix: nil pointer dereference in ALB provider crashes process lb.DNSName and target.Target.Id can be nil for internal/failed ALBs. Dereferencing without check at alb.go:66 panics in a nested goroutine, crashing the entire Aurora pod and leaving enumerations stuck in queued. Confirmed via prod crash log: goroutine 4511917 panicked in listELBV2Resources at alb.go:66 for Group1001 customer (user 349180). * fix: nil pointer dereference in Classic ELB provider Same pattern as ALB — lb.DNSName and instance.InstanceId can be nil. * fix: add panic recovery to all AWS provider goroutines A panic in any nested goroutine (e.g., nil pointer from unexpected AWS API response) crashes the entire process. Add defer/recover to worker() and all GetResource goroutines so panics are logged but don't kill the process. Root cause of Group1001 incident: alb.go goroutine panicked, no recovery existed, Aurora pod crashed, enumeration stuck in queued. * chore: remove investigation debug logging and temp files Remove [cloudlist-debug] prints from aws.go and instances.go, clean up debug variables (workerCount, totalGoroutines) added during Group1001 incident investigation. * chore: remove investigation debug logging and temp files Remove [cloudlist-debug] prints from aws.go and instances.go, clean up debug variables (workerCount, totalGoroutines) added during Group1001 incident investigation. * chore: remove plan docs * fix: address PR review — propagate worker panics, fix ec2Client nil check - Resources() now logs worker errors and returns error when all workers fail instead of silently swallowing panic-derived errors - Inner GetResource goroutines collect panics into error slice instead of just logging — returns error if all goroutines failed - elb.go: fix wrong nil check (ep.elbClient → ec2Client parameter) - recovery_test.go: add t.Parallel() to all tests * fix: track successful workers instead of inferring from resource count
1 parent 83a3b71 commit da6b66e

15 files changed

Lines changed: 407 additions & 1 deletion

.gitignore

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,4 @@ test-*.yaml
1414
/cloudlist
1515
/terraform/
1616
azure-readonly-config.yaml
17+
docs/superpowers/

pkg/providers/aws/alb.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ func (ep *elbV2Provider) GetResource(ctx context.Context) (*schema.Resources, er
3333
list := schema.NewResources()
3434
var wg sync.WaitGroup
3535
var mu sync.Mutex
36+
var errs []error
3637

3738
for _, region := range ep.regions.Regions {
3839
albClients, ec2Clients := ep.getElbV2AndEc2Clients(region.RegionName)
@@ -41,6 +42,13 @@ func (ep *elbV2Provider) GetResource(ctx context.Context) (*schema.Resources, er
4142

4243
go func(albClient *elbv2.ELBV2, ec2Client *ec2.EC2) {
4344
defer wg.Done()
45+
defer func() {
46+
if r := recover(); r != nil {
47+
mu.Lock()
48+
errs = append(errs, fmt.Errorf("panic in alb provider: %v", r))
49+
mu.Unlock()
50+
}
51+
}()
4452

4553
if resources, err := ep.listELBV2Resources(albClient, ec2Client); err == nil {
4654
mu.Lock()
@@ -51,6 +59,9 @@ func (ep *elbV2Provider) GetResource(ctx context.Context) (*schema.Resources, er
5159
}
5260
}
5361
wg.Wait()
62+
if len(errs) > 0 && len(list.Items) == 0 {
63+
return nil, fmt.Errorf("alb: all workers failed: %v", errs)
64+
}
5465
return list, nil
5566
}
5667

@@ -63,6 +74,9 @@ func (ep *elbV2Provider) listELBV2Resources(albClient *elbv2.ELBV2, ec2Client *e
6374
}
6475

6576
for _, lb := range loadBalancers {
77+
if lb.DNSName == nil || lb.LoadBalancerName == nil {
78+
continue
79+
}
6680
albDNS := *lb.DNSName
6781

6882
// Extract metadata for this load balancer
@@ -101,6 +115,9 @@ func (ep *elbV2Provider) listELBV2Resources(albClient *elbv2.ELBV2, ec2Client *e
101115
}
102116

103117
for _, target := range targets.TargetHealthDescriptions {
118+
if target.Target == nil || target.Target.Id == nil {
119+
continue
120+
}
104121
instanceID := *target.Target.Id
105122
instanceOutput, err := ec2Client.DescribeInstances(&ec2.DescribeInstancesInput{
106123
InstanceIds: []*string{&instanceID},

pkg/providers/aws/alb_test.go

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,109 @@
1+
package aws
2+
3+
import (
4+
"testing"
5+
6+
"github.com/aws/aws-sdk-go/aws"
7+
"github.com/aws/aws-sdk-go/service/elbv2"
8+
"github.com/projectdiscovery/cloudlist/pkg/schema"
9+
"github.com/stretchr/testify/assert"
10+
"github.com/stretchr/testify/require"
11+
)
12+
13+
// processLoadBalancersForTest mirrors the nil-safe loop in listELBV2Resources
14+
// so we can exercise the guard logic without a live AWS client.
15+
func processLoadBalancersForTest(lbs []*elbv2.LoadBalancer) *schema.Resources {
16+
list := schema.NewResources()
17+
for _, lb := range lbs {
18+
if lb.DNSName == nil || lb.LoadBalancerName == nil {
19+
continue
20+
}
21+
list.Append(&schema.Resource{
22+
Provider: "aws",
23+
ID: *lb.LoadBalancerName,
24+
DNSName: *lb.DNSName,
25+
Public: true,
26+
Service: "alb",
27+
})
28+
}
29+
return list
30+
}
31+
32+
// processTargetsForTest mirrors the nil-safe target loop in listELBV2Resources.
33+
func processTargetsForTest(targets []*elbv2.TargetHealthDescription) []string {
34+
var ids []string
35+
for _, target := range targets {
36+
if target.Target == nil || target.Target.Id == nil {
37+
continue
38+
}
39+
ids = append(ids, *target.Target.Id)
40+
}
41+
return ids
42+
}
43+
44+
func TestListELBV2Resources_NilDNSName(t *testing.T) {
45+
t.Parallel()
46+
47+
lbs := []*elbv2.LoadBalancer{
48+
{
49+
DNSName: nil,
50+
LoadBalancerName: aws.String("internal-lb"),
51+
LoadBalancerArn: aws.String("arn:aws:elasticloadbalancing:us-east-1:123456789:loadbalancer/app/internal-lb/abc"),
52+
},
53+
{
54+
DNSName: aws.String(""),
55+
LoadBalancerName: nil,
56+
},
57+
}
58+
59+
require.NotPanics(t, func() {
60+
resources := processLoadBalancersForTest(lbs)
61+
assert.Equal(t, 0, len(resources.Items), "nil DNSName or LoadBalancerName LBs must be skipped")
62+
})
63+
}
64+
65+
func TestListELBV2Resources_ValidLB(t *testing.T) {
66+
t.Parallel()
67+
68+
lbs := []*elbv2.LoadBalancer{
69+
{
70+
DNSName: aws.String("my-lb-1234567890.us-east-1.elb.amazonaws.com"),
71+
LoadBalancerName: aws.String("my-lb"),
72+
LoadBalancerArn: aws.String("arn:aws:elasticloadbalancing:us-east-1:123456789:loadbalancer/app/my-lb/abc"),
73+
},
74+
{
75+
DNSName: nil,
76+
LoadBalancerName: aws.String("broken-lb"),
77+
},
78+
}
79+
80+
resources := processLoadBalancersForTest(lbs)
81+
require.Equal(t, 1, len(resources.Items))
82+
assert.Equal(t, "my-lb-1234567890.us-east-1.elb.amazonaws.com", resources.Items[0].DNSName)
83+
assert.Equal(t, "my-lb", resources.Items[0].ID)
84+
}
85+
86+
func TestListELBV2Resources_NilTargetId(t *testing.T) {
87+
t.Parallel()
88+
89+
targets := []*elbv2.TargetHealthDescription{
90+
{
91+
Target: nil,
92+
},
93+
{
94+
Target: &elbv2.TargetDescription{
95+
Id: nil,
96+
},
97+
},
98+
{
99+
Target: &elbv2.TargetDescription{
100+
Id: aws.String("i-0abc123def456"),
101+
},
102+
},
103+
}
104+
105+
require.NotPanics(t, func() {
106+
ids := processTargetsForTest(targets)
107+
assert.Equal(t, []string{"i-0abc123def456"}, ids)
108+
})
109+
}

pkg/providers/aws/aws.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -435,6 +435,11 @@ type result struct {
435435
type getResourcesFunc func(context.Context) (*schema.Resources, error)
436436

437437
func worker(ctx context.Context, fn getResourcesFunc, ch chan<- result) {
438+
defer func() {
439+
if r := recover(); r != nil {
440+
ch <- result{resources: nil, err: fmt.Errorf("panic in provider worker: %v", r)}
441+
}
442+
}()
438443
resources, err := fn(ctx)
439444
ch <- result{resources, err}
440445
}
@@ -497,17 +502,26 @@ func (p *Provider) Resources(ctx context.Context) (*schema.Resources, error) {
497502
assignWorker(cloudfrontProvider.GetResource)
498503
}
499504

505+
500506
go func() {
501507
workersWaitGroup.Wait()
502508
close(results)
503509
}()
504510

511+
var errs []error
512+
successfulWorkers := 0
505513
for result := range results {
506514
if result.err != nil {
515+
gologger.Warning().Msgf("provider worker error: %v", result.err)
516+
errs = append(errs, result.err)
507517
continue
508518
}
519+
successfulWorkers++
509520
finalResources.Merge(result.resources)
510521
}
522+
if len(errs) > 0 && successfulWorkers == 0 {
523+
return finalResources, fmt.Errorf("all provider workers failed: %v", errs)
524+
}
511525
return finalResources, nil
512526
}
513527

pkg/providers/aws/cloudfront.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -31,12 +31,20 @@ func (cp *cloudfrontProvider) GetResource(ctx context.Context) (*schema.Resource
3131
list := schema.NewResources()
3232
var wg sync.WaitGroup
3333
var mu sync.Mutex
34+
var errs []error
3435

3536
for _, client := range cp.getCloudfrontClients() {
3637
wg.Add(1)
3738

3839
go func(cloudfrontClient *cloudfront.CloudFront) {
3940
defer wg.Done()
41+
defer func() {
42+
if r := recover(); r != nil {
43+
mu.Lock()
44+
errs = append(errs, fmt.Errorf("panic in cloudfront provider: %v", r))
45+
mu.Unlock()
46+
}
47+
}()
4048

4149
if resources, err := cp.listCloudFrontResources(cloudfrontClient); err == nil {
4250
mu.Lock()
@@ -46,6 +54,9 @@ func (cp *cloudfrontProvider) GetResource(ctx context.Context) (*schema.Resource
4654
}(client)
4755
}
4856
wg.Wait()
57+
if len(errs) > 0 && len(list.Items) == 0 {
58+
return nil, fmt.Errorf("cloudfront: all workers failed: %v", errs)
59+
}
4960
return list, nil
5061
}
5162

pkg/providers/aws/ecs.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ func (ep *ecsProvider) GetResource(ctx context.Context) (*schema.Resources, erro
3333
list := schema.NewResources()
3434
var wg sync.WaitGroup
3535
var mu sync.Mutex
36+
var errs []error
3637

3738
for _, region := range ep.regions.Regions {
3839
ecsClients, ec2Clients := ep.getEcsAndEc2Clients(region.RegionName)
@@ -41,6 +42,13 @@ func (ep *ecsProvider) GetResource(ctx context.Context) (*schema.Resources, erro
4142

4243
go func(ecsClient *ecs.ECS, ec2Client *ec2.EC2) {
4344
defer wg.Done()
45+
defer func() {
46+
if r := recover(); r != nil {
47+
mu.Lock()
48+
errs = append(errs, fmt.Errorf("panic in ecs provider: %v", r))
49+
mu.Unlock()
50+
}
51+
}()
4452
if resources, err := ep.listECSResources(ecsClient, ec2Client); err == nil {
4553
mu.Lock()
4654
list.Merge(resources)
@@ -50,6 +58,9 @@ func (ep *ecsProvider) GetResource(ctx context.Context) (*schema.Resources, erro
5058
}
5159
}
5260
wg.Wait()
61+
if len(errs) > 0 && len(list.Items) == 0 {
62+
return nil, fmt.Errorf("ecs: all workers failed: %v", errs)
63+
}
5364
return list, nil
5465
}
5566

pkg/providers/aws/eks.go

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,20 @@ func (ep *eksProvider) GetResource(ctx context.Context) (*schema.Resources, erro
3939
list := schema.NewResources()
4040
var wg sync.WaitGroup
4141
var mu sync.Mutex
42+
var errs []error
4243
for _, region := range ep.regions.Regions {
4344
for _, eksClient := range ep.getEksClients(region.RegionName) {
4445
wg.Add(1)
4546

4647
go func(client *eks.EKS) {
4748
defer wg.Done()
49+
defer func() {
50+
if r := recover(); r != nil {
51+
mu.Lock()
52+
errs = append(errs, fmt.Errorf("panic in eks provider: %v", r))
53+
mu.Unlock()
54+
}
55+
}()
4856
if resources, err := ep.listEKSResources(client); err == nil {
4957
mu.Lock()
5058
list.Merge(resources)
@@ -54,6 +62,9 @@ func (ep *eksProvider) GetResource(ctx context.Context) (*schema.Resources, erro
5462
}
5563
}
5664
wg.Wait()
65+
if len(errs) > 0 && len(list.Items) == 0 {
66+
return nil, fmt.Errorf("eks: all workers failed: %v", errs)
67+
}
5768
return list, nil
5869
}
5970

pkg/providers/aws/elb.go

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ func (ep *elbProvider) GetResource(ctx context.Context) (*schema.Resources, erro
3232
list := schema.NewResources()
3333
var wg sync.WaitGroup
3434
var mu sync.Mutex
35+
var errs []error
3536

3637
for _, region := range ep.regions.Regions {
3738
elbClients, ec2Clients := ep.getElbAndEc2Clients(region.RegionName)
@@ -41,6 +42,13 @@ func (ep *elbProvider) GetResource(ctx context.Context) (*schema.Resources, erro
4142

4243
go func(elbClient *elb.ELB, ec2Client *ec2.EC2) {
4344
defer wg.Done()
45+
defer func() {
46+
if r := recover(); r != nil {
47+
mu.Lock()
48+
errs = append(errs, fmt.Errorf("panic in elb provider: %v", r))
49+
mu.Unlock()
50+
}
51+
}()
4452

4553
if resources, err := ep.listELBResources(elbClient, ec2Client); err == nil {
4654
mu.Lock()
@@ -51,6 +59,9 @@ func (ep *elbProvider) GetResource(ctx context.Context) (*schema.Resources, erro
5159
}
5260
}
5361
wg.Wait()
62+
if len(errs) > 0 && len(list.Items) == 0 {
63+
return nil, fmt.Errorf("elb: all workers failed: %v", errs)
64+
}
5465
return list, nil
5566
}
5667

@@ -63,6 +74,9 @@ func (ep *elbProvider) listELBResources(elbClient *elb.ELB, ec2Client *ec2.EC2)
6374
}
6475

6576
for _, lb := range loadBalancerDescriptions {
77+
if lb.DNSName == nil || lb.LoadBalancerName == nil {
78+
continue
79+
}
6680
elbDNS := *lb.DNSName
6781

6882
// Extract metadata for this load balancer
@@ -81,11 +95,14 @@ func (ep *elbProvider) listELBResources(elbClient *elb.ELB, ec2Client *ec2.EC2)
8195
}
8296
list.Append(resource)
8397

84-
if ep.elbClient == nil {
98+
if ec2Client == nil {
8599
continue
86100
}
87101
// Describe Instances for the Load Balancer
88102
for _, instance := range lb.Instances {
103+
if instance.InstanceId == nil {
104+
continue
105+
}
89106
instanceID := *instance.InstanceId
90107
instanceOutput, err := ec2Client.DescribeInstances(&ec2.DescribeInstancesInput{
91108
InstanceIds: []*string{&instanceID},

0 commit comments

Comments
 (0)