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

SIGSEGV on closure / stack heavy program #11091

Closed
recmo opened this issue Jan 31, 2017 · 11 comments
Closed

SIGSEGV on closure / stack heavy program #11091

recmo opened this issue Jan 31, 2017 · 11 comments
Labels
invalid Issues and PRs that are invalid. v8 engine Issues and PRs related to the V8 dependency.

Comments

@recmo
Copy link

recmo commented Jan 31, 2017

Source code: async-fizzbuzz.js

$ node --stack-size=100000 ./async-fizzbuzz.js
1
2
Fizz
4
Buzz
[…]
Fizz
88
89
FizzBuzz
fish: “node --stack-size=100000 ./asyn…” terminated by signal SIGSEGV (Address boundary error)
@recmo recmo changed the title SegFault on closure / stack heavy program SIGSEGV on closure / stack heavy program Jan 31, 2017
@addaleax addaleax added confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency. labels Jan 31, 2017
@addaleax
Copy link
Member

This reproduces for me with d8 on V8’s master for me. Could you open an issue at https://bugs.chromium.org/p/v8/issues and link that here?

@recmo
Copy link
Author

recmo commented Jan 31, 2017

v8 bugreport.

@mscdex
Copy link
Contributor

mscdex commented Feb 1, 2017

FWIW it might be better to include the code from the gist inline (but collapsed) in case the gist goes away.

@addaleax
Copy link
Member

@nodejs/v8 Looks like the v8 issue was closed, but this seems like a valid bug report to me… can one of you take a look?

@bnoordhuis
Copy link
Member

Works for me with master on OS X, with or without --stack-size=100000.

@targos
Copy link
Member

targos commented Feb 18, 2017

Same as @bnoordhuis for me, on Fedora 25.

@gibfahn
Copy link
Member

gibfahn commented Feb 19, 2017

FWIW it might be better to include the code from the gist inline (but collapsed) in case the gist goes away.

EDIT: This is the fixed version, see below.

async-fizzbuzz.js
// Async FizzBuzz -- implemented using only callbacks
//
// node --stack-size=100000 ./async-fizzbuzz.js
//

// Wrap process.stdout.write
const print = (message, callback) =>{
	process.stdout.write(message, callback)
}

// Scott numeral constructors
const Z = (callback)=>{callback((a,b)=>{a()})}
const S = (n, callback)=>{callback((a,b)=>{b(n)})}

// Numeral constants
const n0 = (a,b)=>{a()}
const n1 = (a,b)=>{b(n0)}
const n2 = (a,b)=>{b(n1)}
const n3 = (a,b)=>{b(n2)}
const n4 = (a,b)=>{b(n3)}
const n5 = (a,b)=>{b(n4)}
const n6 = (a,b)=>{b(n5)}
const n7 = (a,b)=>{b(n6)}
const n8 = (a,b)=>{b(n7)}
const n9 = (a,b)=>{b(n8)}
const n10 = (a,b)=>{b(n9)}

const if_zero = (n, then, else_)=>{n(
	()=>{then()},
	(r)=>{else_()}
)}

const if_equal = (m, n, then, else_)=>{m(
	()=>{if_zero(n, then, else_)},
	(mr)=>{n(
		else_,
		(nr)=>{if_equal(mr, nr, then, else_)}
	)}
)}

const add = (m, n, callback)=>{m(
	()=>{callback(n)},
	(mr)=>{S(n, (sn)=>{add(mr, sn, callback)})}
)}

const sub = (m, n, callback, error)=>{n(
	()=>{callback(m)},
	(nr)=>{m(
		error,
		(mr)=>{sub(mr, nr, callback, error)}
	)}
)}

const mul = (m, n, callback)=>{m(
	()=>{Z(callback)},
	(mr)=>{mul(mr, n, (r)=>{add(r, n, callback)})}
)}

const divmod = (m, n, callback)=>{
	sub(m, n,
		(mr)=>{divmod(mr, n, (q, r)=>{
			S(q, (sq)=>{callback(sq, r)})
		})}, 
		()=>{callback(n0, m)})
}

const divides = (m, n, then, else_)=>{
	divmod(m, n, (q, r)=>{if_equal(r, n0, then, else_)})
}

// Decimal output

const print_decimal = (n, callback, error)=>{
	if_equal(n, n0, ()=>{print("0", callback)}, ()=>{
	if_equal(n, n1, ()=>{print("1", callback)}, ()=>{
	if_equal(n, n2, ()=>{print("2", callback)}, ()=>{
	if_equal(n, n3, ()=>{print("3", callback)}, ()=>{
	if_equal(n, n4, ()=>{print("4", callback)}, ()=>{
	if_equal(n, n5, ()=>{print("5", callback)}, ()=>{
	if_equal(n, n6, ()=>{print("6", callback)}, ()=>{
	if_equal(n, n7, ()=>{print("7", callback)}, ()=>{
	if_equal(n, n8, ()=>{print("8", callback)}, ()=>{
	if_equal(n, n9, ()=>{print("9", callback)}, error
	)})})})})})})})})}) // LISP flashbacks
}

const print_number = (n, callback)=>{
	divmod(n, n10, (q, r)=>{
		if_zero(q, ()=>{
			print_decimal(r, callback)
		}, ()=>{
			print_number(q, ()=>{
				print_decimal(r, callback)
			})
		})
	})
}

// Loops

// for needs more callbacks.
// So far, we have only used anonymous functions. It would
// be a shame to stop that practice here. Therefore, I present
// you a fixed-point combinator in pure CPS:
const for_ = (start, end, func, callback)=>{
	((rec, n, end, func, callback)=>{
		rec(rec, n, end, func, callback)
	})((rec, n, end, func, callback)=>{
		func(n, ()=>{
			if_equal(n, end, callback, ()=>{
				S(n, (sn)=>{
					rec(rec, sn, end, func, callback)
				})
			})
		})
	}, start, end, func, callback)
}

// We are coming to the main show

const fizzbuzz_number = (n, callback)=>{mul(n3, n5, (n15)=>{
	divides(n, n15, ()=>{print("FizzBuzz\n", callback)}, ()=>{
	divides(n, n3, ()=>{print("Fizz\n", callback)}, ()=>{
	divides(n, n5, ()=>{print("Buzz\n", callback)}, ()=>{
	print_number(n, ()=>{print("\n", callback)})
	})})})
})}

const fizzbuzz = (callback)=>{mul(n10, n10, (n100)=>{
	for_(n1, n100, fizzbuzz_number, callback)
})}

fizzbuzz(()=>{})

@recmo
Copy link
Author

recmo commented Feb 19, 2017

@gibfahn The gist was updated which causes the bug to no longer be triggered. The snippet you posted is the updated version and works for me. I suspect @targos and @bnoordhuis also ran this version. (It's not hard to make that one crash again, this is described in the Gist comments.) I apologize for the confusion.

Here's a hard link to the original breaking version and here's the code of the breaking version:

Edit: Sorry, I can't figure out the GitHub-Markdown syntax for collapsing.

// Async FizzBuzz — implemented using *only* callbacks
//
// node --stack-size=100000 ./FizzBuzz.js
//

// We will need print function with a callback
var output = "";
const print = (message, callback)=>{
	output += message;
	callback();
}
const flush = (callback)=>{
	console.log(output);
	output = "";
	callback()
}

// Scott numeral constructors
const Z = (callback)=>{callback((a,b)=>{a()})}
const S = (n, callback)=>{callback((a,b)=>{b(n)})}

// Numeral constants
const n0 = (a,b)=>{a()}
const n1 = (a,b)=>{b(n0)}
const n2 = (a,b)=>{b(n1)}
const n3 = (a,b)=>{b(n2)}
const n4 = (a,b)=>{b(n3)}
const n5 = (a,b)=>{b(n4)}
const n6 = (a,b)=>{b(n5)}
const n7 = (a,b)=>{b(n6)}
const n8 = (a,b)=>{b(n7)}
const n9 = (a,b)=>{b(n8)}
const n10 = (a,b)=>{b(n9)}

const if_zero = (n, then, else_)=>{n(
	()=>{then()},
	(r)=>{else_()}
)}

const if_equal = (m, n, then, else_)=>{m(
	()=>{if_zero(n, then, else_)},
	(mr)=>{n(
		else_,
		(nr)=>{if_equal(mr, nr, then, else_)}
	)}
)}

const add = (m, n, callback)=>{m(
	()=>{callback(n)},
	(mr)=>{S(n, (sn)=>{add(mr, sn, callback)})}
)}

const sub = (m, n, callback, error)=>{n(
	()=>{callback(m)},
	(nr)=>{m(
		error,
		(mr)=>{sub(mr, nr, callback, error)}
	)}
)}

const mul = (m, n, callback)=>{m(
	()=>{Z(callback)},
	(mr)=>{mul(mr, n, (r)=>{add(r, n, callback)})}
)}

const divmod = (m, n, callback)=>{
	sub(m, n,
		(mr)=>{divmod(mr, n, (q, r)=>{
			S(q, (sq)=>{callback(sq, r)})
		})}, 
		()=>{callback(n0, m)})
}

const divides = (m, n, then, else_)=>{
	divmod(m, n, (q, r)=>{if_equal(r, n0, then, else_)})
}

// Decimal output

const print_decimal = (n, callback, error)=>{
	if_equal(n, n0, ()=>{print("0", callback)}, ()=>{
	if_equal(n, n1, ()=>{print("1", callback)}, ()=>{
	if_equal(n, n2, ()=>{print("2", callback)}, ()=>{
	if_equal(n, n3, ()=>{print("3", callback)}, ()=>{
	if_equal(n, n4, ()=>{print("4", callback)}, ()=>{
	if_equal(n, n5, ()=>{print("5", callback)}, ()=>{
	if_equal(n, n6, ()=>{print("6", callback)}, ()=>{
	if_equal(n, n7, ()=>{print("7", callback)}, ()=>{
	if_equal(n, n8, ()=>{print("8", callback)}, ()=>{
	if_equal(n, n9, ()=>{print("9", callback)}, error
	)})})})})})})})})}) // LISP flashbacks
}

const print_number = (n, callback)=>{
	divmod(n, n10, (q, r)=>{
		if_zero(q, ()=>{
			print_decimal(r, callback)
		}, ()=>{
			print_number(q, ()=>{
				print_decimal(r, callback)
			})
		})
	})
}

// Loops


// for needs more callbacks.
// We need recursion, so anonymous functions are not enough here
// TODO: Can this be done with anonymous functions?
// https://stackoverflow.com/questions/35918279/define-fix-point-combinator-in-continuation-passing-style#
const for_ = (start, end, func, callback)=>{
	(function rec(n) {
		func(n, ()=>{
			if_equal(n, end, callback, ()=>{
				S(n, (sn)=>{rec(sn)})
			})
		})
	})(start)
}

// We are coming to the main show

const fizzbuzz_number = (n, callback)=>{mul(n3, n5, (n15)=>{
	divides(n, n15, ()=>{print("FizzBuzz", callback)}, ()=>{
	divides(n, n3, ()=>{print("Fizz", callback)}, ()=>{
	divides(n, n5, ()=>{print("Buzz", callback)}, ()=>{
	print_number(n, callback)
	})})})
})}

const fizzbuzz_number_newline = (n, callback)=>{
	fizzbuzz_number(n, ()=>{flush(callback)})
}

const fizzbuzz = (callback)=>{mul(n10, n10, (n100)=>{
	for_(n1, n100, fizzbuzz_number_newline, callback)
})}

fizzbuzz(()=>{})

@bnoordhuis
Copy link
Member

I can reproduce now but it's not a bug, it's a misunderstanding about what --stack-size=... does. It tells V8 to assume - not create - the stack is that big. The help message for the option mentions that.

Your test informs it the stack is approx. 100 MB big when in reality it's probably only around 8 MB. Set ulimit -s 10000 first and it should work. :-)

@bnoordhuis bnoordhuis added invalid Issues and PRs that are invalid. and removed confirmed-bug Issues with confirmed bugs. labels Feb 20, 2017
@recmo
Copy link
Author

recmo commented Feb 23, 2017

@bnoordhuis Confirmed. ulimit -s 10000; node --stack-size=100000 ./async-fizzbuzz.js works!

The stack-size option is not in the man page or in the --help. I got it from this stackoverflow post where it is introduced as increasing the available stack.

After some digging I found that node --v8-options has --stack_size (note the underscore) and the cryptic/ambiguous "default size of stack region v8 is allowed to use", which can be interpreted as you say.

Perhaps node should use getrlimitor setrlimit for the stacksize and not allow foot-shooting this way?

@bnoordhuis
Copy link
Member

It's not out the question but it opens a can of worms. Resizing the stack is complicated on Windows and needs some decisions in general: should node exit with an error when resizing fails or do we silently fix up --stack_size to a reasonable value? If so, what constitutes 'a reasonable value' and do we print a warning?

And so on - there is probably more that I haven't thought of. You are welcome to have a go at it, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid Issues and PRs that are invalid. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

6 participants