net/url: Panic in (nil).ResolveReference(…) #22229
Comments
|
I don't see anything wrong with a panic here. Don't call |
|
On Thu, Oct 12, 2017 at 04:37:11PM +0000, Ian Lance Taylor wrote:
I don't see anything wrong with a panic here. Don't call
`ResolveReference` on a `nil` `URL`. That rule is no different than
"don't dereference a nil pointer."
I don't think it's that clearcut, because as the absoluteReference
resolution in my example [1] shows, a nil base *works* when the
reference is absolute (and no information from the base is needed).
Labeling the cases:
a. Real base, sufficiently absolute reference: resolved URI matches
the initial reference.
b. nil base, sufficiently absolute reference: resolved URI matches
the initial reference.
c. Real base, relative reference: resolved URI combines components
from both input URIs.
d. nil base, relative reference: currently panics, but I'd like to see
nil returned instead.
e. Real base, nil relative reference: currently panics, and I'm fine
with either panics or a returned nil.
f. nil base, nil relative reference: currently panics, and I'm fine
with either panics or a returned nil.
(d) is this issue, and (b) is a nil-base that doesn't panic in master.
What I'm looking for is a way to cleanly error (not panicking) in case
(d). There's a semi-real-world example in wking/casengine@4d8f93e8.
I guess I could recover the panic, but I don't want to accidentally
recover from a *different* panic.
[1]: https://play.golang.org/p/grCVrmNOxz
|
|
I don't think we should return Looking at the code, it seems that |
I had to read it over a few times, but yeah, that looks right. Thanks :) Cross-linking #20924 as a similar issue, but for different methods, in case anyone stumbles across the other while searching. |
…base Avoiding the situation discussed in 4d8f93e (template: Document unbased ResolveReference panic, 2017-10-12). In the Go bug I'd filed, Ian pointed out that (nil).ResolveReference panics if and only if the reference scheme is nil, and that IsAbs is only looking at the scheme [1]. So this commit guards against data that would panic ResolveReference, instead of catching and handling an error/panic that ResolveReference raises. I've also added the base URI to the testcase struct, so we can exercise different values there. In this case, I needed to use just the encoded part to get the relative-reference check to raise an error at all, because some-algorithm:0123456789abcdef matches the URI syntax with a 'some-algorithm' scheme and '0123456789abcdef' hier-part [2]. That looks (rightly) absolute, so ResolveReference wasn't looking at the nil base. By using just the encoded part, we no longer have the colon required for an absolute URI. [1]: golang/go#22229 (comment) [2]: https://tools.ietf.org/html/rfc3986#section-3 [3]: https://tools.ietf.org/html/rfc3986#section-4.2 Signed-off-by: W. Trevor King <wking@tremily.us>
Please answer these questions before submitting your issue. Thanks!
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes.
What operating system and processor architecture are you using (
go env)?This bug is OS-agnostic.
What did you do?
https://play.golang.org/p/grCVrmNOxz
What did you expect to see?
ResolveReferencedoesn't return an error, so the best we can do is probably returnnilif the argument isnilor the argument is relative-enough that we'd need to pull anything off the base object.What did you see instead?
A panic, but only in cases where the argument is nil, or when the argument is relative-enough that we'd need to pull something of a
nilbase object.The text was updated successfully, but these errors were encountered: