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

services/horizon/internal/ingest: Fix nil pointer dereference in captive core metrics #4824

Merged
merged 2 commits into from
Mar 29, 2023

Conversation

tamirms
Copy link
Contributor

@tamirms tamirms commented Mar 28, 2023

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

There is a bug where horizon panics with a nil dereference when running Horizon with remote captive core enabled.
The nil deref occurs on this line:

if !s.config.EnableCaptiveCore || (s.config.CaptiveCoreToml.HTTPPort == 0)

When remote captive core is enabled s.config.EnableCaptiveCore is true but s.config.CaptiveCoreToml is nil since the toml file is only fore local captive core.

Known limitations

[N/A]

@tamirms tamirms requested a review from a team March 28, 2023 15:31
Copy link
Contributor

@tsachiherman tsachiherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if s.config.CaptiveCoreToml could be a nil value, the better solution would be to test that explicitly instead of assuming the relationship between that and s.config.RemoteCaptiveCoreURL.

i.e.

if !s.config.EnableCaptiveCore || // captive core is disabled
   (s.config.CaptiveCoreToml != nil && s.config.CaptiveCoreToml.HTTPPort == 0) { // captive core http port is disabled
	return -1
}

@tamirms
Copy link
Contributor Author

tamirms commented Mar 29, 2023

@tsachiherman @sreuland

EnableCaptiveCore implies we're either using a local captive core or a remote captive core.
s.config.RemoteCaptiveCoreURL != "" is only true if we're using a remote captive core.
s.config.CaptiveCoreToml is guaranteed to be non-nil if we're using a local captive core but it will be nil if we're not using captive core or we're using a remote captive core

Copy link
Contributor

@sreuland sreuland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for doing that additional refactor to clarify remote vs local captive core

@tamirms tamirms merged commit 2446dc2 into stellar:master Mar 29, 2023
@tamirms tamirms deleted the fix-metrics-bug branch March 29, 2023 17:28
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.

4 participants