Skip to content
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

syscall: unix Sendto panic if nil Sockaddr provided #55845

Closed
nathanaelle opened this issue Sep 24, 2022 · 5 comments
Closed

syscall: unix Sendto panic if nil Sockaddr provided #55845

nathanaelle opened this issue Sep 24, 2022 · 5 comments
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@nathanaelle
Copy link

What version of Go are you using (go version)?

1.19.1

Does this issue reproduce with the latest release?

yes.

What operating system and processor architecture are you using (go env)?

ubuntu linux 22.04 on amd64

What did you do?

syscall.Sendto(socket, buffer, flag, nil)

What did you expect to see?

as specified in https://linux.die.net/man/2/sendto

send(sockfd, buf, len, flags);

is equivalent to

sendto(sockfd, buf, len, flags, NULL, 0); 

What did you see instead?

we can see in https://cs.opensource.google/go/go/+/refs/tags/go1.19.1:src/syscall/syscall_unix.go;l=445

func Sendto(fd int, p []byte, flags int, to Sockaddr) (err error) {
	ptr, n, err := to.sockaddr()
	if err != nil {
		return err
	}
	return sendto(fd, p, flags, ptr, n)
}

the nil situation for to is not tested, thus a panic() occurs.

Suggestion

is it possible to add a test to avoid a panic() when to is set to nil like in SendmsgN() ( see: https://cs.opensource.google/go/go/+/refs/tags/go1.19.1:src/syscall/syscall_unix.go;l=400 ) ?

this issue may also impact :
– SendtoInet4
– SendtoInet6
– SendmsgNInet4
– SendmsgNInet6

@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 24, 2022
@nathanaelle nathanaelle changed the title syscall: unix Sendto panic if nil addr provided syscall: unix Sendto panic if nil Sockaddr provided Sep 24, 2022
@tklauser
Copy link
Member

I think it would make sense to add the check for syscall.Sendto. The other functions (SendtoInet4, SendtoInet6, SendmsgNInet4, SendmsgNInet6) are defined in internal/syscall/unix and are only used internally by Go. As far as I can see all call sites pass a non-nil Sockaddr, so I'm not sure we'd need the check there.

/cc @ianlancetaylor

@tklauser tklauser added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 24, 2022
@tklauser tklauser modified the milestones: Unplanned, Backlog Sep 24, 2022
@ianlancetaylor ianlancetaylor added help wanted NeedsFix The path to resolution is known, but the work has not been done. and removed compiler/runtime Issues related to the Go compiler and/or runtime. labels Sep 25, 2022
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 25, 2022
@gopherbot
Copy link

Change https://go.dev/cl/433504 mentions this issue: syscall: check if dest_addr is nil to prevent panic from Sendto

@gopherbot
Copy link

Change https://go.dev/cl/434515 mentions this issue: unix: allow calling Sendto with nil Sockaddr

@gopherbot
Copy link

Change https://go.dev/cl/434535 mentions this issue: syscall: check if to is nil to prevent panic from WSASendto

gopherbot pushed a commit to golang/sys that referenced this issue Sep 26, 2022
Same as CL 433504 did for the syscall package.

For golang/go#55845

Change-Id: I3ca2d7fab9da77c304487b4c134a9675da99deca
Reviewed-on: https://go-review.googlesource.com/c/sys/+/434515
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Auto-Submit: Ian Lance Taylor <iant@google.com>
@dmitshur dmitshur modified the milestones: Backlog, Go1.20 Sep 26, 2022
gopherbot pushed a commit that referenced this issue Sep 26, 2022
to is an optional pointer to sockaddr, as written in the doc:
https://learn.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-wsasendto

For #55845

Change-Id: Ia685cec8d9bc9ff313f598db9d2213a1f409757a
Reviewed-on: https://go-review.googlesource.com/c/go/+/434535
Reviewed-by: Ian Lance Taylor <iant@google.com>
Auto-Submit: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: xie cui <523516579@qq.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Ian Lance Taylor <iant@google.com>
Reviewed-by: Tobias Klauser <tobias.klauser@gmail.com>
@gopherbot
Copy link

Change https://go.dev/cl/435777 mentions this issue: windows: allow calling WSASendto with nil Sockaddr

gopherbot pushed a commit to golang/sys that referenced this issue Sep 28, 2022
Same as CL 434535 did for the syscall package.

For golang/go#55845

Change-Id: I17f30152ae973b64ac65e08cefd5442e9bf19e2c
Reviewed-on: https://go-review.googlesource.com/c/sys/+/435777
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Benny Siegert <bsiegert@gmail.com>
Auto-Submit: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Benny Siegert <bsiegert@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
@golang golang locked and limited conversation to collaborators Sep 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants