proj icon indicating copy to clipboard operation
proj copied to clipboard

Handle null pointer when attempting to build an error string

Open urschrei opened this issue 3 years ago • 8 comments

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

urschrei avatar Feb 13 '21 11:02 urschrei

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

https://github.com/georust/proj/blob/81b233a54f2da49b5e5a5ae1aee855910df838ec/src/proj.rs#L133-L139

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 avatar Feb 13 '21 16:02 frewsxcv

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

urschrei avatar Feb 13 '21 16:02 urschrei

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.

frewsxcv avatar Feb 13 '21 16:02 frewsxcv

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

urschrei avatar Aug 16 '21 09:08 urschrei

bors try

urschrei avatar Aug 16 '21 09:08 urschrei

try

Build failed:

bors[bot] avatar Aug 16 '21 09:08 bors[bot]

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.

urschrei avatar Aug 16 '21 15:08 urschrei

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.

michaelkirk avatar Aug 17 '21 22:08 michaelkirk