Skip to content

IPv6 literals in remotes triggers SIGSEGV #8703

Closed
#8893
Closed
IPv6 literals in remotes triggers SIGSEGV#8703
#8893
Labels
bugSomething isn't workinghelp wantedContributions welcomepriority-3Affects a small number of users or is largely cosmetic

Description

@jcgruenhage
jcgruenhage
opened on Feb 15, 2024

Describe the bug

Having an IPv6 literal in a remote url causes SIGSEGV in gh pr create.

❯ gh --version
gh version 2.43.1 (2024-02-01)
https://github.com/cli/cli/releases/tag/v2.43.1

Steps to reproduce the behavior

  1. Clone some repo from github
  2. Add a remote that utilizes an IPv6 literal, like ssh://git@[::1]:22/tmp/git-repo
  3. Run gh pr create
  4. Witness SIGSEGV

Expected vs actual behavior

I'd expect this to work without a segfault, even if there's a remote using IPv6 literals.

Logs

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x30 pc=0x7d0e91]

goroutine 1 [running]:
github.com/cli/go-gh/v2/pkg/ssh.(*Translator).Translate(0x0?, 0xc0006aa090)
	/builddir/github-cli-2.43.1/_build-github-cli-xbps/pkg/mod/github.com/cli/go-gh/[email protected]/pkg/ssh/ssh.go:43 +0xb1
github.com/cli/cli/v2/context.TranslateRemotes({0xc0007326a0, 0x2, 0xc000822e00?}, {0x2479aa0, 0xc00068c660})
	/builddir/github-cli-2.43.1/context/remote.go:109 +0x89
github.com/cli/cli/v2/pkg/cmd/factory.New.remotesFunc.(*remoteResolver).Resolver.func6()
	/builddir/github-cli-2.43.1/pkg/cmd/factory/remote_resolver.go:49 +0x105
github.com/cli/cli/v2/pkg/cmd/pr/create.getRemotes(0xc000002d80)
	/builddir/github-cli-2.43.1/pkg/cmd/pr/create/create.go:667 +0x18
github.com/cli/cli/v2/pkg/cmd/pr/create.NewCreateContext(0xc000002d80)
	/builddir/github-cli-2.43.1/pkg/cmd/pr/create/create.go:515 +0x88
github.com/cli/cli/v2/pkg/cmd/pr/create.createRun(0xc000002d80)
	/builddir/github-cli-2.43.1/pkg/cmd/pr/create/create.go:224 +0x3b
github.com/cli/cli/v2/pkg/cmd/pr/create.NewCmdCreate.func1(0xc0002bba00?, {0x2dca240?, 0x4?, 0x15095eb?})
	/builddir/github-cli-2.43.1/pkg/cmd/pr/create/create.go:187 +0x786
github.com/spf13/cobra.(*Command).execute(0xc0002f5200, {0x2dca240, 0x0, 0x0})
	/builddir/github-cli-2.43.1/_build-github-cli-xbps/pkg/mod/github.com/spf13/[email protected]/command.go:916 +0x87c
github.com/spf13/cobra.(*Command).ExecuteC(0xc000004300)
	/builddir/github-cli-2.43.1/_build-github-cli-xbps/pkg/mod/github.com/spf13/[email protected]/command.go:1044 +0x3a5
github.com/spf13/cobra.(*Command).ExecuteContextC(...)
	/builddir/github-cli-2.43.1/_build-github-cli-xbps/pkg/mod/github.com/spf13/[email protected]/command.go:977
main.mainRun()
	/builddir/github-cli-2.43.1/cmd/gh/main.go:119 +0x5db
main.main()
	/builddir/github-cli-2.43.1/cmd/gh/main.go:46 +0x13

Activity

  1. jcgruenhage
    added
    bugSomething isn't working
    on Feb 15, 2024
  2. cliAutomation
    added
    needs-triageneeds to be reviewed
    on Feb 15, 2024

  3. williammartin commented on Feb 15, 2024

    @williammartin
    williammartin
    on Feb 15, 2024
    Member

    The error being ignored here looks awfully suspicious: https://github.com/cli/go-gh/blob/d88d88f917cd4e0bcaa582bac2bb909bd79f9759/pkg/ssh/ssh.go#L42

    I don't have time to investigate this further right now but I would welcome someone to come take a look at this.

  4. williammartin
    added
    priority-3Affects a small number of users or is largely cosmetic
    help wantedContributions welcome
    and removed
    needs-triageneeds to be reviewed
    on Feb 15, 2024

  5. babakks commented on Mar 30, 2024

    @babakks
    babakks
    on Mar 30, 2024
    Member

    @williammartin Thanks for the hint. I just pushed a PR to fix this, which just touches the code in this repo.

    As a side note regarding the masked error, I think we can improve the Translate method's signature to also return an error. This way, we don't need to ignore the error and we can just return it to the caller. I expect the changes to both cli/go-gh and cli/cli to be minimal (of course, unless there's no other project/code that depends on the same method), but I don't know if it's okay with you. Please let me know if you think we should do that, and I'll submit further PRs.

  6. williammartin commented on Apr 5, 2024

    @williammartin
    williammartin
    on Apr 5, 2024
    Member

    @babakks I am always in favour of bubbling errors and being explicit about handling them, even if the behaviour remains the same. See also here:

    cli/context/remote.go

    Lines 109 to 112 in 2c53d7c

    repo, _ = ghrepo.FromURL(translator.Translate(r.FetchURL))
    }
    if r.PushURL != nil && repo == nil {
    repo, _ = ghrepo.FromURL(translator.Translate(r.PushURL))

    The first return value is being used as a proxy for the error cases. It's extremely confusing because it twists successful cases and error cases into one, and makes it harder to change later; you have to go and read the implementation of the FromURL function to understand whether error cases are always the same as nil in the first return. This has been a source of a number of bugs I've discovered.

    However, we have a bit of an issue because changing the signature is technically a breaking change in go-gh. Being pragmatic, I don't think anyone is really going to be bothered if we changed it in a minor version but I could be wrong. I'm not even sure anyone else uses this package aside from cli/cli. 🤔

  7. williammartin
    closed this as completedin #8893on Apr 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't workinghelp wantedContributions welcomepriority-3Affects a small number of users or is largely cosmetic

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions