Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix panic in ExtractIntoStructPtr #2873

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

Merged
merged 1 commit into from
Jan 30, 2025
Merged

Conversation

bobuhiro11
Copy link
Contributor

Passing nil as an argument to 'ExtractIntoStructPtr' causes "panic: runtime error: invalid memory address or nil pointer dereference". This is an invalid argument and should be corrected to return an error. For reference, standard package functions such as 'json.Unmarshal' return an error instead of panic.

Fixes #2872

@github-actions github-actions bot added the semver:patch No API change label Jan 19, 2024
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for submitting your first PR! Be sure that we will be looking at it but keep in mind
this sometimes takes a while.
Please let the maintainers know if your PR has not got enough attention after a few days.
If any doubt, please consult our PR tutorial.

@bobuhiro11 bobuhiro11 marked this pull request as ready for review January 19, 2024 08:00
@pierreprinetti pierreprinetti self-assigned this Jan 19, 2024
@coveralls
Copy link

coveralls commented Jan 19, 2024

Coverage Status

coverage: 77.875% (+0.04%) from 77.832%
when pulling 571d3f5 on bobuhiro11:fix_extract
into d0c1d7e on gophercloud:master.

@pierreprinetti
Copy link
Member

The comment to the function you are modifying specifically mentions that it's "for internal use only". However I realise that since it's exported, it's integral part of the API surface.

I think your change request has merit and it's correctly written. Can you please do the same for ExtractIntoSlicePtr just below?

Copy link
Contributor

@kayrus kayrus left a comment

Choose a reason for hiding this comment

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

@bobuhiro11 thanks for the PR. I think it's also worth adding a check, whether interface is not a pointer. And an error message should be more specific, this should help during the development.

results.go Outdated Show resolved Hide resolved
testing/results_test.go Outdated Show resolved Hide resolved
@pierreprinetti
Copy link
Member

I have a last-minute doubt about this PR. see #2872 (comment)

@github-actions github-actions bot added semver:patch No API change and removed semver:patch No API change labels Jan 19, 2024
Passing nil as an argument to 'ExtractIntoStructPtr' or
'ExtractIntoSlicePtr' causes "panic: runtime error:
invalid memory address or nil pointer dereference".
This is an invalid argument and should be corrected to
return an error. For reference, standard package functions
such as 'json.Unmarshal' return an error instead of panic.

Signed-off-by: Nobuhiro MIKI <nob@bobuhiro11.net>
Co-authored-by: pýrus <kayrus@users.noreply.github.com>
@github-actions github-actions bot added semver:patch No API change and removed semver:patch No API change labels Jan 19, 2024
@bobuhiro11
Copy link
Contributor Author

@pierreprinetti @kayrus
Thanks for your review!

I think your change request has merit and it's correctly written. Can you please do the same for ExtractIntoSlicePtr just below?

Of course. I added the same change for ExtractIntoSlicePtr.

thanks for the PR. I think it's also worth adding a check, whether interface is not a pointer. And an error message should be more specific, this should help during the development.

I appied your suggestions.
reflect.ValueOf(to).IsNil() was needed for err5.

@bobuhiro11
Copy link
Contributor Author

@pierreprinetti @kayrus
Could you please take a look at it again?

Copy link
Member

@pierreprinetti pierreprinetti left a comment

Choose a reason for hiding this comment

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

OK, let's merge this. I was hesitant, but if the standard library goes out of its way to match a literal nil, maybe we can too.

@pierreprinetti
Copy link
Member

@kayrus are you fine with the changes you requested?

@pierreprinetti
Copy link
Member

@kayrus ping?

@bobuhiro11
Copy link
Contributor Author

@kayrus Could you please check?

Copy link
Contributor

@kayrus kayrus left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@EmilienM
Copy link
Contributor

I don't understand why we can't merge it.

@EmilienM EmilienM added hold Do not merge and removed hold Do not merge labels Jan 30, 2025
@EmilienM EmilienM merged commit d03d5d1 into gophercloud:main Jan 30, 2025
3 of 4 checks passed
@EmilienM EmilienM added the backport-v2 This PR will be backported to v2 label Jan 30, 2025
@bobuhiro11
Copy link
Contributor Author

Thank you for your review.

@bobuhiro11 bobuhiro11 deleted the fix_extract branch January 31, 2025 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v2 This PR will be backported to v2 semver:patch No API change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panic in 'ExtractIntoStructPtr'
5 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.