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.

Sign up for GitHub

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

Jump to bottom

SetVal: Fix stack overflow in umarking logic #35

Merged
merged 1 commit into from
Jan 10, 2020
Merged

SetVal: Fix stack overflow in umarking logic #35

merged 1 commit into from
Jan 10, 2020

Conversation

radeksimko
Copy link
Contributor

@radeksimko radeksimko commented Jan 9, 2020 •
edited
Loading

This fixes the following stack overflow as demonstrated in the attached test.

runtime: goroutine stack exceeds 1000000000-byte limit
fatal error: stack overflow

runtime stack:
runtime.throw(0x12c7621, 0xe)
	/Users/radeksimko/.goenv/versions/1.12.9/src/runtime/panic.go:617 +0x72
runtime.newstack()
	/Users/radeksimko/.goenv/versions/1.12.9/src/runtime/stack.go:1041 +0x6f0
runtime.morestack()
	/Users/radeksimko/.goenv/versions/1.12.9/src/runtime/asm_amd64.s:429 +0x8f

...

@apparentlymart apparentlymart mentioned this pull request Jan 9, 2020
Merged
radeksimko
radeksimko commented Jan 10, 2020
View reviewed changes
cty/walk_test.go
gotVal, err := Transform(val, func(path Path, val Value) (Value, error) {
if val.Type().IsPrimitiveType() {
gotVal, err := Transform(val, func(path Path, v Value) (Value, error) {
if v.Type().IsPrimitiveType() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code isn't really affected, but I decided to change it anyway to prevent the ambiguity and similar bugs in the future, in case anyone copies from here.

@radeksimko radeksimko changed the title SetVal with nested SetVal causes stack overflow SetVal: Fix stack overflow in umarking logic Jan 10, 2020
@codecov
Copy link

codecov bot commented Jan 10, 2020 •
edited
Loading

Codecov Report

Merging #35 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #35   +/-   ##
=======================================
  Coverage   76.11%   76.11%           
=======================================
  Files          77       77           
  Lines        6172     6172           
=======================================
  Hits         4698     4698           
  Misses       1058     1058           
  Partials      416      416
Impacted Files Coverage Δ
cty/marks.go 79.57% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update da0d8b6...6cc2c81. Read the comment docs.

@apparentlymart
Copy link
Collaborator

Looks great! Thanks for tracking this down and fixing it. 🎉

@apparentlymart apparentlymart merged commit e362ebe into zclconf:master Jan 10, 2020
@radeksimko radeksimko deleted the b-setval-fix branch January 10, 2020 17:29
@radeksimko radeksimko restored the b-setval-fix branch January 10, 2020 17:29
@radeksimko radeksimko deleted the b-setval-fix branch January 10, 2020 17:29
@meshuga meshuga mentioned this pull request Feb 8, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@radeksimko @apparentlymart