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

pubsub/kafkapub: fix nil pointer dereference #3211

Merged
merged 1 commit into from Jan 15, 2023

Conversation

danielorbach
Copy link
Contributor

ExampleOpenTopic (pubsub/kafkapubsub/example_test.go) show a simple call to OpenTopic without kafkapubsub.TopicOptions as passing a nil argument.
This is done by replacing a nil pointer with the appropriate zero-value. This behaviour exists in OpenSubscription too. However, PR #3163 introduced overriding batching options which inadvertently accesses a field of a nil pointer.

/cc @vangent I see you authored the relevant files, could you please take a look?

P.S> This is my first commit to this repository (read the contributing guidelines though) - so couple of questions:

  1. Would you like me to report a concomitant bug and reference it from the commit's message?
  2. Markdown in commit body - yes/no?

Local testing

$ go test ./...
?       gocloud.dev     [no test files]
ok      gocloud.dev/aws (cached)
ok      gocloud.dev/aws/awscloud        (cached) [no tests to run]
?       gocloud.dev/aws/rds     [no test files]
?       gocloud.dev/azure/azurecloud    [no test files]
?       gocloud.dev/azure/azuredb       [no test files]
ok      gocloud.dev/blob        (cached)
ok      gocloud.dev/blob/azureblob      (cached)
?       gocloud.dev/blob/driver [no test files]
?       gocloud.dev/blob/drivertest     [no test files]
ok      gocloud.dev/blob/fileblob       (cached)
ok      gocloud.dev/blob/gcsblob        (cached)
ok      gocloud.dev/blob/memblob        1.035s
ok      gocloud.dev/blob/s3blob (cached)
ok      gocloud.dev/docstore    (cached)
ok      gocloud.dev/docstore/awsdynamodb        (cached)
ok      gocloud.dev/docstore/driver     (cached)
?       gocloud.dev/docstore/drivertest [no test files]
ok      gocloud.dev/docstore/gcpfirestore       (cached)
ok      gocloud.dev/docstore/internal/fields    (cached)
ok      gocloud.dev/docstore/memdocstore        (cached)
ok      gocloud.dev/gcerrors    0.153s
ok      gocloud.dev/gcp (cached)
?       gocloud.dev/gcp/cloudsql        [no test files]
ok      gocloud.dev/gcp/gcpcloud        (cached) [no tests to run]
ok      gocloud.dev/internal/escape     (cached)
ok      gocloud.dev/internal/gcerr      (cached)
ok      gocloud.dev/internal/oc (cached)
ok      gocloud.dev/internal/openurl    (cached)
ok      gocloud.dev/internal/releasehelper      3.097s
ok      gocloud.dev/internal/retry      (cached)
?       gocloud.dev/internal/testing/octest     [no test files]
?       gocloud.dev/internal/testing/setup      [no test files]
?       gocloud.dev/internal/testing/terraform  [no test files]
ok      gocloud.dev/internal/testing/test-summary       (cached)
?       gocloud.dev/internal/useragent  [no test files]
ok      gocloud.dev/mysql       2.842s
ok      gocloud.dev/mysql/awsmysql      3.133s
ok      gocloud.dev/mysql/azuremysql    2.184s
ok      gocloud.dev/mysql/gcpmysql      0.158s
ok      gocloud.dev/postgres    (cached)
ok      gocloud.dev/postgres/awspostgres        2.520s
ok      gocloud.dev/postgres/gcppostgres        0.822s
ok      gocloud.dev/pubsub      (cached)
ok      gocloud.dev/pubsub/awssnssqs    (cached)
ok      gocloud.dev/pubsub/azuresb      (cached)
ok      gocloud.dev/pubsub/batcher      (cached)
?       gocloud.dev/pubsub/driver       [no test files]
?       gocloud.dev/pubsub/drivertest   [no test files]
ok      gocloud.dev/pubsub/gcppubsub    (cached)
ok      gocloud.dev/pubsub/mempubsub    (cached)
ok      gocloud.dev/runtimevar  (cached)
ok      gocloud.dev/runtimevar/awsparamstore    (cached)
ok      gocloud.dev/runtimevar/awssecretsmanager        (cached)
ok      gocloud.dev/runtimevar/blobvar  (cached)
ok      gocloud.dev/runtimevar/constantvar      (cached)
?       gocloud.dev/runtimevar/driver   [no test files]
?       gocloud.dev/runtimevar/drivertest       [no test files]
ok      gocloud.dev/runtimevar/filevar  (cached)
ok      gocloud.dev/runtimevar/gcpruntimeconfig (cached)
ok      gocloud.dev/runtimevar/gcpsecretmanager (cached)
ok      gocloud.dev/runtimevar/httpvar  (cached)
ok      gocloud.dev/secrets     (cached)
ok      gocloud.dev/secrets/awskms      (cached)
ok      gocloud.dev/secrets/azurekeyvault       (cached)
?       gocloud.dev/secrets/driver      [no test files]
?       gocloud.dev/secrets/drivertest  [no test files]
ok      gocloud.dev/secrets/gcpkms      (cached)
ok      gocloud.dev/secrets/localsecrets        (cached)
ok      gocloud.dev/server      (cached)
?       gocloud.dev/server/driver       [no test files]
ok      gocloud.dev/server/health       (cached)
ok      gocloud.dev/server/health/sqlhealth     (cached)
ok      gocloud.dev/server/requestlog   (cached)
?       gocloud.dev/server/sdserver     [no test files]
?       gocloud.dev/server/xrayserver   [no test files]

`ExampleOpenTopic` (pubsub/kafkapubsub/example_test.go) show a
simple call to `OpenTopic` without `kafkapubsub.TopicOptions` as
passing a `nil` argument.
This is done by replacing a `nil` pointer with the appropriate
zero-value.  This behaviour exists in `OpenSubscription` too.
However, PR google#3163 introduced overriding batching options which
inadvertently accesses a field of a nil pointer.
Copy link
Contributor

@vangent vangent 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 the fix! Looks fine, no need for a bug.

@codecov
Copy link

codecov bot commented Jan 15, 2023

Codecov Report

Merging #3211 (a5b7e6c) into master (664ae9b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3211      +/-   ##
==========================================
- Coverage   72.99%   72.98%   -0.01%     
==========================================
  Files         112      112              
  Lines       14358    14361       +3     
==========================================
+ Hits        10480    10481       +1     
- Misses       3150     3152       +2     
  Partials      728      728              
Impacted Files Coverage Δ
pubsub/kafkapubsub/kafka.go 84.74% <100.00%> (+0.15%) ⬆️
blob/s3blob/s3blob.go 89.43% <0.00%> (-0.28%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@vangent vangent merged commit ec14a9f into google:master Jan 15, 2023
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.

None yet

2 participants