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.

Sign up for GitHub

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

Jump to bottom

Handle null pointer when attempting to build an error string #74

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Handle null pointer when attempting to build an error string #74

wants to merge 4 commits into from

Conversation

urschrei
Copy link
Member

It's possible that libproj will return a null pointer instead of an
actual error string. In this case we now return a specific error
indicating this.

Fixes #73

@frewsxcv
Copy link
Member

frewsxcv commented Feb 13, 2021 •
edited

The primary place that's affected by #73 is:

proj/src/proj.rs

Lines 133 to 139 in 81b233a

/// Look up an error message using the error code
fn error_message(code: c_int) -> Result<String, ProjError> {
unsafe {
let rv = proj_errno_string(code);
_string(rv)
}
}

Specifically, the proj_errno_string function will return a null pointer if a 0 error code is passed to the function. And in such a situation, _string will attempt to dereference the null pointer which is undefined behavior, which we want to avoid.

So the question I'm wondering: do we have a guarantee that PROJ will always generate non-zero error codes?

  • If the answer is 'yes, PROJ will always generate non-zero error codes', then, if we do encounter a zero error code in this function, we can assume there is some bug on our side in the proj crate codebase. And in that case, I'd propose we panic (e.g. add assert_ne!(code, 0)) so the users of our crate know it's a bug that should be reported to us, as opposed to returning an Err(...) which will subtly hide the bug.

  • If the answer is 'no, PROJ will sometimes generate zero error codes', then this pull request seems good to me as we would want to surface a non-panicking error that users of this crate can gracefully handle.

frewsxcv
frewsxcv approved these changes Feb 13, 2021
View reviewed changes
Copy link
Member

@frewsxcv frewsxcv left a comment

Choose a reason for hiding this comment

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

Can we add a regression test for this? Could be as simple as:

#[test]
fn error_message_zero_code() {
    assert!(error_message(0).is_err());
}

I don't want my previous message to block this as I think your pull request as-is is an improvement! Just wanted to get that thought out, as I'm not sure what the answer is. We can discuss it in a separate GitHub Issue if that's preferred

@urschrei
Copy link
Member Author

OK, I'll ask the authors to clarify the error code issue, and then let's see where we are.

@frewsxcv
Copy link
Member

Awesome, thanks for doing that. Depending on the answer, I think there are some improvements I/we/they could make to the upstream PROJ documentation to clarify as it's currently ambiguous.

@urschrei
Copy link
Member Author

The smallest error code that PROJ can (should) return is 1024, so anything smaller than that should be a panic.

@urschrei
Copy link
Member Author

bors try

bors bot added a commit that referenced this pull request Aug 16, 2021
@bors
Try #74:
a8902d7
@bors
Copy link
Contributor

bors bot commented Aug 16, 2021

try

Build failed:

@urschrei
Handle null pointer when attempting to build an error string
dbb86b4 
It's possible that libproj will return a null pointer instead of an
actual error string. In this case we now return a specific error
indicating this.

Fixes georust#73
@urschrei
Remove null pointer check in favour of assert
3b57482
@urschrei
Copy link
Member Author

For some reason the proj@7 formula doesn't seem to play nicely with pkg-config, so gonna revert that change and hold off on merging this until the update to PROJ 8.1.0 has landed.

@michaelkirk
Copy link
Member

For some reason the proj@7 formula doesn't seem to play nicely with pkg-config

Hmmm... I'm pretty sure I had the proj@7 working with the proj crate on my mac, but I'm also happy to wait until the whole pipeline is updated to proj8.

LMK if you'd like me to test anything locally on my mac.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frewsxcv frewsxcv frewsxcv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handle a null return value for proj_sys::proj_errno_string in the proj crate
3 participants
@urschrei @frewsxcv @michaelkirk