Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] [exporterhelper] Fix nil ptr dereference on storage stop #8718

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

fredthomsen
Copy link
Contributor

When persistent queue fails to start, for example due to bad permissions, then persistent storage does not get allocated. Thus, ensure stop storage method is only called if actually initialized.

Description:
Fixing a bug that resulted in panic when de-referencing nil pointer. The issue is that in the persistent queue fail to start due to say bad folder permissions, then the Start method does not actually set the associated persistent storage, and thus when shutting down it tries to stop the storage and panics.

The traceback appeared as follows:

otc-otc-1  | 2023-10-21T05:10:03.187Z   info    extensions/extensions.go:33     Starting extensions...
otc-otc-1  | 2023-10-21T05:10:03.187Z   info    extensions/extensions.go:36     Extension is starting...        {"kind": "extension", "name": "file_storage/sl1receiver"}
otc-otc-1  | 2023-10-21T05:10:03.187Z   info    extensions/extensions.go:43     Extension started.      {"kind": "extension", "name": "file_storage/sl1receiver"}
otc-otc-1  | 2023-10-21T05:10:03.187Z   info    extensions/extensions.go:36     Extension is starting...        {"kind": "extension", "name": "file_storage/exporter_queue"}
otc-otc-1  | 2023-10-21T05:10:03.187Z   info    extensions/extensions.go:43     Extension started.      {"kind": "extension", "name": "file_storage/exporter_queue"}
otc-otc-1  | 2023-10-21T05:10:03.187Z   info    extensions/extensions.go:36     Extension is starting...        {"kind": "extension", "name": "memory_ballast"}
otc-otc-1  | 2023-10-21T05:10:03.388Z   info    [email protected]/memory_ballast.go:41   Setting memory ballast  {"kind": "extension", "name": "memory_ballast", "MiBs": 6419}
otc-otc-1  | 2023-10-21T05:10:03.389Z   info    extensions/extensions.go:43     Extension started.      {"kind": "extension", "name": "memory_ballast"}
otc-otc-1  | 2023-10-21T05:10:03.390Z   info    [email protected]/service.go:178  Starting shutdown...
otc-otc-1  | panic: runtime error: invalid memory address or nil pointer dereference
otc-otc-1  | [signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0xeb5782]
otc-otc-1  |
otc-otc-1  | goroutine 1 [running]:
otc-otc-1  | go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*persistentContiguousStorage).stop(0x0)
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/exporterhelper/internal/persistent_storage.go:196 +0x22
otc-otc-1  | go.opentelemetry.io/collector/exporter/exporterhelper/internal.glob..func1(0xc000a90540?)
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/exporterhelper/internal/persistent_queue.go:19 +0x17
otc-otc-1  | go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*persistentQueue).Stop.func1()
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/exporterhelper/internal/persistent_queue.go:94 +0x42
otc-otc-1  | sync.(*Once).doSlow(0xc000b5b128?, 0xc000b5b0e0?)
otc-otc-1  |    sync/once.go:74 +0xbf
otc-otc-1  | sync.(*Once).Do(...)
otc-otc-1  |    sync/once.go:65
otc-otc-1  | go.opentelemetry.io/collector/exporter/exporterhelper/internal.(*persistentQueue).Stop(0x38dd560?)
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/exporterhelper/internal/persistent_queue.go:90 +0x3c
otc-otc-1  | go.opentelemetry.io/collector/exporter/exporterhelper.(*queueSender).shutdown(0xc00092a3f0)
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/exporterhelper/queue_sender.go:161 +0x78
otc-otc-1  | go.opentelemetry.io/collector/exporter/exporterhelper.(*baseExporter).Shutdown(0xc00052e000, {0x4c563b0, 0x72ffec0})
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/exporterhelper/common.go:221 +0x4d
otc-otc-1  | go.opentelemetry.io/collector/service/internal/graph.(*Graph).ShutdownAll(0xc000c547e0, {0x4c563b0, 0x72ffec0})
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/internal/graph/graph.go:422 +0x1c3
otc-otc-1  | go.opentelemetry.io/collector/service.(*Service).Shutdown(0xc000a3dc20, {0x4c563b0, 0x72ffec0})
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/service.go:184 +0xd7
otc-otc-1  | go.opentelemetry.io/collector/otelcol.(*Collector).setupConfigurationComponents(0xc000b3a000, {0x4c563b0, 0x72ffec0})
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/collector.go:187 +0x6ca
otc-otc-1  | go.opentelemetry.io/collector/otelcol.(*Collector).Run(0xc000b3a000, {0x4c563b0, 0x72ffec0})
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/collector.go:221 +0x52
otc-otc-1  | go.opentelemetry.io/collector/otelcol.NewCommand.func1(0xc000b24000, {0x436812e?, 0x7?, 0x436307f?})
otc-otc-1  |    go.opentelemetry.io/collector/[email protected]/command.go:27 +0x74
otc-otc-1  | github.com/spf13/cobra.(*Command).execute(0xc000b24000, {0xc000072050, 0x1, 0x1})
otc-otc-1  |    github.com/spf13/[email protected]/command.go:940 +0x87c
otc-otc-1  | github.com/spf13/cobra.(*Command).ExecuteC(0xc000b24000)
otc-otc-1  |    github.com/spf13/[email protected]/command.go:1068 +0x3a5
otc-otc-1  | github.com/spf13/cobra.(*Command).Execute(0xc0006eb110?)
otc-otc-1  |    github.com/spf13/[email protected]/command.go:992 +0x13
otc-otc-1  | main.runInteractive({{0xc0006eb110, 0xc000b200c0, 0xc0006eb200, 0xc0006eb0e0, 0xc000b200f0}, {{0x436fd4c, 0xa}, {0x43d9ac3, 0x24}, {0x436598d, ...}}, ...})
otc-otc-1  |    go.opentelemetry.io/collector/cmd/builder/main.go:31 +0x45
otc-otc-1  | main.run(...)
otc-otc-1  |    go.opentelemetry.io/collector/cmd/builder/main_others.go:11
otc-otc-1  | main.main()
otc-otc-1  |    go.opentelemetry.io/collector/cmd/builder/main.go:24 +0x1d8

Testing:

  • Added unit test
  • Tested by setting up a directory w/file_storage extension that did not have the necessary permissions.

@fredthomsen fredthomsen requested review from a team and dmitryax October 21, 2023 06:03
@codecov
Copy link

codecov bot commented Oct 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Files Coverage Δ
...porter/exporterhelper/internal/persistent_queue.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

When persistent queue fails to start, for example due to bad
permissions, then persistent storage does not get allocated.
Thus, ensure stop storage method is only called if actually
initialized.
@fredthomsen fredthomsen changed the title [fix] [exporterhelper] Fix nil ptr dereference [fix] [exporterhelper] Fix nil ptr dereference on storage stop Oct 21, 2023
@dmitryax dmitryax merged commit 37116a2 into open-telemetry:main Oct 23, 2023
32 checks passed
@github-actions github-actions bot added this to the next release milestone Oct 23, 2023
@fredthomsen fredthomsen deleted the fixNilPtrStorageCrash branch October 23, 2023 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants