Skip to content

Fix nil pointer dereference in Collector.handleOnError#887

Open
Shinku-Chen wants to merge 1 commit into
gocolly:masterfrom
Shinku-Chen:Fix-nil-pointer-dereference-in-Collector.handleOnError
Open

Fix nil pointer dereference in Collector.handleOnError#887
Shinku-Chen wants to merge 1 commit into
gocolly:masterfrom
Shinku-Chen:Fix-nil-pointer-dereference-in-Collector.handleOnError

Conversation

@Shinku-Chen

@Shinku-Chen Shinku-Chen commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Collector.handleOnError dereferenced response.StatusCode before
the nil-guard that synthesized a fallback Response. Short-circuit
evaluation of err == nil && ... masked the bug whenever err was
non-nil, but the err == nil && response == nil path — the exact
case the existing nil-guard was clearly written to defend against —
panicked with runtime error: invalid memory address or nil pointer dereference.

This MR reorders the function so the nil-guard runs first, ensuring
every subsequent field access (including the debugger event that
reads response.StatusCode) operates on a valid Response.

Changes

  • colly.go: move the response == nil check to the top of
    handleOnError, before any response.StatusCode access.
  • colly_test.go: add TestHandleOnErrorNilResponse with two
    subtests:
    • nil response with transport error(nil, err) returned by
      the backend on DNS/connection failures.
    • nil response with nil error — reproduces the original panic;
      fails on master, passes with this fix.

Test plan

  • go test -run TestHandleOnErrorNilResponse -v . passes.
  • go test ./... — full suite green.
  • Verified the new test panics without the fix (stashed
    colly.go, re-ran, observed
    runtime error: invalid memory address or nil pointer dereference).

Risk

Low. The reordering is behavior-preserving for all paths that
previously did not panic: when response != nil, the new code takes
the same branches as before. The only behavioral change is that
calls with response == nil no longer crash and instead proceed
with the synthesized Response the original author had already
prepared.

Backward compatibility

No API changes. No exported signatures modified.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant