-
Notifications
You must be signed in to change notification settings - Fork 335
Fixed bug in DTEDRasterReader where AVList was assumed non-null #7
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
Conversation
…it, if null create a new empty AVListImpl.
I was wondering if you could provide an example or use case of an instance where a null AVList would be provided by the caller? While checking for a null AVList does avoid a potential null pointer exception, the newly created AVList drops out of scope on DTEDRasterReader return. If you could provide some additional details or an example of the situation we can work from there on a solution. Thanks! |
I don't have an example where it is valid or desirable necessarily, but for RaptorX (raptorx.org) there was a case where we were calling into the logic with "null" for some reason. I have fixed it on our end, but I thought this might be an easy robustness improvement. |
I was talking with @pdavidc about this and he pointed out there is a null check for setting the params to the AVList on line 61 of the current method. So it does seem a null check is missing at the point you identified. If you are up for it, I'll merge the request if you add a null check before the first call to the params.setValue method. With the params null check on line 61 and, any new instantiation of AVList dropping out of scope at the end of the method, I don't think instantiating a new AVList is required, what about you? Let me know if you have any questions or if I missed something. Please match the formatting of the existing code to include the bracket positioning and order of null check if statements. Thanks @mikeoertli for the catch! We appreciate your effort to make WWJ better! |
I've got a big push for a project through 8/4, should be able to address this shortly after. At a glance, I agree with what you've outlined, thanks for the feedback. |
No longer instantiating a new AVList, slight tweak to the null check for the params list.
This reverts commit d0a3cd1.
Thanks @mikeoertli ! |
Sorry for the whole pile of commits there, was having issues with my git client and forgot to clean that up before pushing. I think everything we'd talked about is in there, but if I Mondayed it up, let me know :). |
@mikeoertli, wanted to thank you again for identifying and working with us on a solution to this issue. One of the motivations for moving to Github was facilitation of community feedback and coordination. I'm modifying your approach slightly and pushing changes this evening. Please feel free to reach out in the future with other issues you find! |
PS: in open source development it usually is better to accept such a pull request and then commit code changes to it to have a correct changelog and attribution of who did which changes and for what reason. Otherwise you will loose new developers from the community quickly. |
In DTEDRasterReader added check for null AVList before trying to use it, if null create a new empty AVListImpl.