[go] multiple: fix lock management bugs and improve error handling
0 views
Skip to first unread message
Gerrit Bot (Gerrit)
10:14 AM (5 hours ago) 10:14 AM
to goph...@pubsubhelper.golang.org, feizaizheli, golang-co...@googlegroups.com
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
multiple: fix lock management bugs and improve error handling
This commit fixes several critical bugs across multiple packages:
1. syscall: Fix incorrect defer usage in forkpipe2.go lock management
- acquireForkLock: Remove defer that causes double unlock when hasWaitingReaders returns true. The deferred Unlock() would execute after manual unlock/relock sequence, causing undefined behavior.
- releaseForkLock: Unlock before panic to prevent deadlock when detecting negative forking count.
2. encoding/json: Add defensive bounds checking in stream.go
- Token(): Add len(tokenStack) == 0 check before accessing tokenStack[len(tokenStack)-1] for both ']' and '}' delimiters.
- Prevents potential panic on malformed or concurrent access.
3. syscall: Replace panic with error return in Accept4 functions
- syscall_freebsd.go, syscall_netbsd.go, syscall_openbsd.go:
- Change panic("RawSockaddrAny too small") to return EINVAL error.
- Allows graceful error handling instead of crashing the program.
These fixes improve robustness in concurrent scenarios and abnormal input handling, following Go's error handling best practices.
Fixes: Lock management issues in fork operations
Fixes: Potential array bounds violations in JSON parsing
Fixes: Inappropriate panic usage in syscall package
This PR will be imported into Gerrit with the title and first
comment (this text) used to generate the subject and body of
the Gerrit change.
**Please ensure you adhere to every item in this list.**
More info can be found at https://github.com/golang/go/wiki/CommitMessage
+ The PR title is formatted as follows: `net/http: frob the quux before blarfing`
+ The package name goes before the colon
+ The part after the colon uses the verb tense + phrase that completes the blank in,
"This change modifies Go to ___________"
+ Lowercase verb after the colon
+ No trailing period
+ Keep the title as short as possible. ideally under 76 characters or shorter
+ No Markdown
+ The first PR comment (this one) is wrapped at 76 characters, unless it's
really needed (ASCII art, table, or long link)
+ If there is a corresponding issue, add either `Fixes #1234` or `Updates #1234`
(the latter if this is not a complete fix) to this comment
+ If referring to a repo other than `golang/go` you can use the
`owner/repo#issue_number` syntax: `Fixes golang/tools#1234`
+ We do not use Signed-off-by lines in Go. Please don't add them.
Our Gerrit server & GitHub bots enforce CLA compliance instead.
+ Delete these instructions once you have read and applied them
Change-Id: Ia1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0
GitHub-Last-Rev: 387ee2524212deb9334496c9f75bce1fcdec6c0f
GitHub-Pull-Request: golang/go#77848
Change diff
diff --git a/src/encoding/json/stream.go b/src/encoding/json/stream.go
index fc480c9..c1c86cd 100644
--- a/src/encoding/json/stream.go
+++ b/src/encoding/json/stream.go
@@ -387,6 +387,9 @@
if dec.tokenState != tokenArrayStart && dec.tokenState != tokenArrayComma {
return dec.tokenError(c)
}
+ if len(dec.tokenStack) == 0 {
+ return dec.tokenError(c)
+ }
dec.scanp++
dec.tokenState = dec.tokenStack[len(dec.tokenStack)-1]
dec.tokenStack = dec.tokenStack[:len(dec.tokenStack)-1]
@@ -406,6 +409,9 @@
if dec.tokenState != tokenObjectStart && dec.tokenState != tokenObjectComma {
return dec.tokenError(c)
}
+ if len(dec.tokenStack) == 0 {
+ return dec.tokenError(c)
+ }
dec.scanp++
dec.tokenState = dec.tokenStack[len(dec.tokenStack)-1]
dec.tokenStack = dec.tokenStack[:len(dec.tokenStack)-1]
diff --git a/src/syscall/forkpipe2.go b/src/syscall/forkpipe2.go
index bbecfda..fcaa112 100644
--- a/src/syscall/forkpipe2.go
+++ b/src/syscall/forkpipe2.go
@@ -38,12 +38,12 @@
// at the first fork and unlocked when there are no more forks.
func acquireForkLock() {
forkingLock.Lock()
- defer forkingLock.Unlock()
if forking == 0 {
// There is no current write lock on ForkLock.
ForkLock.Lock()
forking++
+ forkingLock.Unlock()
return
}
@@ -77,15 +77,16 @@
}
forking++
+ forkingLock.Unlock()
}
// releaseForkLock releases the conceptual write lock on ForkLock
// acquired by acquireForkLock.
func releaseForkLock() {
forkingLock.Lock()
- defer forkingLock.Unlock()
if forking <= 0 {
+ forkingLock.Unlock()
panic("syscall.releaseForkLock: negative count")
}
@@ -95,4 +96,6 @@
// No more conceptual write locks.
ForkLock.Unlock()
}
+
+ forkingLock.Unlock()
}
diff --git a/src/syscall/syscall_freebsd.go b/src/syscall/syscall_freebsd.go
index 584522d..d672e65 100644
--- a/src/syscall/syscall_freebsd.go
+++ b/src/syscall/syscall_freebsd.go
@@ -109,7 +109,8 @@
return
}
if len > SizeofSockaddrAny {
- panic("RawSockaddrAny too small")
+ Close(nfd)
+ return 0, nil, EINVAL
}
sa, err = anyToSockaddr(&rsa)
if err != nil {
diff --git a/src/syscall/syscall_netbsd.go b/src/syscall/syscall_netbsd.go
index 5a239f8..c2b1262 100644
--- a/src/syscall/syscall_netbsd.go
+++ b/src/syscall/syscall_netbsd.go
@@ -139,7 +139,8 @@
return
}
if len > SizeofSockaddrAny {
- panic("RawSockaddrAny too small")
+ Close(nfd)
+ return 0, nil, EINVAL
}
sa, err = anyToSockaddr(&rsa)
if err != nil {
diff --git a/src/syscall/syscall_openbsd.go b/src/syscall/syscall_openbsd.go
index 80a3854..abe77ac 100644
--- a/src/syscall/syscall_openbsd.go
+++ b/src/syscall/syscall_openbsd.go
@@ -94,7 +94,8 @@
return
}
if len > SizeofSockaddrAny {
- panic("RawSockaddrAny too small")
+ Close(nfd)
+ return 0, nil, EINVAL
}
sa, err = anyToSockaddr(&rsa)
if err != nil {
Change information
Files:
- M src/encoding/json/stream.go
- M src/syscall/forkpipe2.go
- M src/syscall/syscall_freebsd.go
- M src/syscall/syscall_netbsd.go
- M src/syscall/syscall_openbsd.go
Change size: S
Delta: 5 files changed, 17 insertions(+), 5 deletions(-)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gopher Robot (Gerrit)
10:14 AM (5 hours ago) 10:14 AM
to feizaizheli, Gerrit Bot, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gopher Robot added 1 comment![]( "Open in Gerrit")
Patchset-level comments
File-level comment, Patchset 1 (Latest):
Gopher Robot . unresolved
I spotted some possible problems with your PR:
1. Do you still have the GitHub PR instructions in your commit message text? The PR instructions should be deleted once you have applied them.
2. Do you have the right bug reference format? For this repo, the format is usually 'Fixes #12345' or 'Updates #12345' at the end of the commit message.
Please address any problems by updating the GitHub PR.
When complete, mark this comment as 'Done' and click the [blue 'Reply' button](https://go.dev/wiki/GerritBot#i-left-a-reply-to-a-comment-in-gerrit-but-no-one-but-me-can-see-it) above. These findings are based on heuristics; if a finding does not apply, briefly reply here saying so.
To update the commit title or commit message body shown here in Gerrit, you must edit the GitHub PR title and PR description (the first comment) in the GitHub web interface using the 'Edit' button or 'Edit' menu entry there. Note: pushing a new commit to the PR will not automatically update the commit message used by Gerrit.
For more details, see:
- [how to update commit messages](https://go.dev/wiki/GerritBot/#how-does-gerritbot-determine-the-final-commit-message) for PRs imported into Gerrit.
- the Go project's [conventions for commit messages](https://go.dev/doc/contribute#commit_messages) that you should follow.
(In general for Gerrit code reviews, the change author is expected to [log in to Gerrit](https://go-review.googlesource.com/login/) with a Gmail or other Google account and then close out each piece of feedback by marking it as 'Done' if implemented as suggested or otherwise reply to each review comment. See the [Review](https://go.dev/doc/contribute#review) section of the Contributing Guide for details.)
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
10:24 AM (5 hours ago) 10:24 AM
to feizaizheli, goph...@pubsubhelper.golang.org, golang-co...@googlegroups.com
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Gerrit Bot uploaded patch set #2 to this change.
Related details
Attention set is empty
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
11:37 AM (4 hours ago) 11:37 AM
to feizaizheli, Gerrit Bot, goph...@pubsubhelper.golang.org, Keith Randall, Brad Fitzpatrick, Ian Lance Taylor, Tobias Klauser, Gopher Robot, golang-co...@googlegroups.com
Attention needed from Brad Fitzpatrick, Ian Lance Taylor and Tobias Klauser
Keith Randall added 3 comments![]( "Open in Gerrit")
Commit Message
Line 9, Patchset 2 (Latest):
This commit fixes several critical bugs across multiple packages:Keith Randall . unresolved
Please use separate change lists for independent changes.
Line 12, Patchset 2 (Latest):
- acquireForkLock: Remove defer that causes double unlock when hasWaitingReaders returns true. The deferred Unlock() would execute after manual unlock/relock sequence, causing undefined behavior.Keith Randall . unresolved
The original code looks correct to me. There is no risk of double unlock. The unlock in the hasWaitingReaders case has a paired lock.
File src/encoding/json/stream.go
Line 387, Patchset 2 (Latest):
if dec.tokenState != tokenArrayStart && dec.tokenState != tokenArrayComma {Keith Randall . unresolved
I believe this test ensures that the token stack is nonempty.
Related details
Attention is currently required from:
- Brad Fitzpatrick
- Ian Lance Taylor
- Tobias Klauser
Submit Requirements:
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Reply all
Reply to author
Forward
0 new messages