Skip to content

fix modulo by zero panic - #235

Open
youdie006 wants to merge 1 commit into
CloudyKit:masterfrom
youdie006:fix-modulo-by-zero-panic
Open

youdie006 wants to merge 1 commit into
CloudyKit:masterfrom
youdie006:fix-modulo-by-zero-panic

Conversation

@youdie006

Copy link
Copy Markdown

Problem

{{ a % b }} with a zero divisor panics with Go's runtime error: integer divide by zero, and the panic escapes Template.Execute and kills the caller's process. The sibling / in the same switch already returns a clean jet runtime error.

eval.go:1025-1034 (pristine) reaches the % operator with no guard:

	case itemMod:
		if isInt(kind) {
			left = reflect.ValueOf(left.Int() % toInt(right))
		} else if isFloat(kind) {
			left = reflect.ValueOf(int64(left.Float()) % toInt(right))
		} else if isUint(kind) {
			left = reflect.ValueOf(left.Uint() % toUint(right))
		} else {

This is not merely uncaught, it is deliberately not caught. (*Runtime).recover at eval.go:237-241 re-panics anything that is a runtime.Error:

	if recovered := recover(); recovered != nil {
		var ok bool
		if _, ok = recovered.(runtime.Error); ok {
			panic(recovered)
		}

so an operator that can produce one has to guard before it runs. That is what case itemDiv: does at eval.go:1004-1007.

Reachability

Run against 607e931 through the public Template.Execute, with the caller's own recover() around the call so an escaped panic is visible:

{{ n / zero }}     -> EXEC-ERR: Jet Runtime Error ("/t":1): division by zero
{{ n % zero }}     -> PANIC-ESCAPED-Template.Execute: runtime error: integer divide by zero
{{ 10 % 0 }}       -> PANIC-ESCAPED-Template.Execute: runtime error: integer divide by zero
{{ u % 0 }}        -> PANIC-ESCAPED-Template.Execute: runtime error: integer divide by zero
{{ 10.5 % 0.0 }}   -> PANIC-ESCAPED-Template.Execute: runtime error: integer divide by zero
{{ 10 % half }}    -> PANIC-ESCAPED-Template.Execute: runtime error: integer divide by zero

n, zero, u and half are ordinary VarMap variables, so this is reachable from template data, not just from a literal in the template source. After the fix each of those returns Jet Runtime Error ("/t":N): modulo by zero, and every in-range result is unchanged.

Why this is a bug and not a design choice

Your own case itemDiv: at eval.go:1005-1007 already does this - it rejects a zero divisor with node.Left.errorf("division by zero") so the caller gets a catchable jet runtime error - while case itemMod: twelve lines below does not.

That guard was added in f1947cd ("fix division by zero panic"), whose message says "division by zero now results in a runtime error instead of a panic". % divides too, and was left as it was.

Fix

 	case itemMod:
+		// the divisor is truncated to an integer, so a non-zero fraction is a zero divisor too
 		if isInt(kind) {
-			left = reflect.ValueOf(left.Int() % toInt(right))
+			divisor := toInt(right)
+			if divisor == 0 {
+				node.Left.errorf("modulo by zero")
+			}
+			left = reflect.ValueOf(left.Int() % divisor)
 		} else if isFloat(kind) {
-			left = reflect.ValueOf(int64(left.Float()) % toInt(right))
+			divisor := toInt(right)
+			if divisor == 0 {
+				node.Left.errorf("modulo by zero")
+			}
+			left = reflect.ValueOf(int64(left.Float()) % divisor)
 		} else if isUint(kind) {
-			left = reflect.ValueOf(left.Uint() % toUint(right))
+			divisor := toUint(right)
+			if divisor == 0 {
+				node.Left.errorf("modulo by zero")
+			}
+			left = reflect.ValueOf(left.Uint() % divisor)
 		} else {

The one design point worth stating: copying itemDiv's guard literally as if right.IsZero() is not enough. % has no float form, so the divisor goes through toInt/toUint and is truncated - {{ 10 % 0.5 }} still reaches % 0 and still panics. The guard has to sit on the converted divisor, which is why it is inside the three branches rather than above them.

Test

TestModuloByZero in eval_test.go, next to the existing TestDivisionByZero and asserting the same way: 8 zero-divisor templates that must return an error containing modulo by zero, plus 5 in-range templates that must keep their current output. It uses a slice with subtests rather than the map the neighbour uses, because on the unfixed code the first case aborts the test binary and the ordering has to be deterministic.

Red / green / mutation, go test -count=1 -run TestModuloByZero . each time:

state exit first failing subtest
as shipped, new test 1 int_modulo_by_zero - panic: runtime error: integer divide by zero [recovered, repanicked] at eval.go:1029, repanicked by (*Runtime).recover at eval.go:240. Unrecoverable by the caller, so no later subtest runs
this PR 0 none, 13/13 pass
under-correct: guard right.IsZero() only 1 int_modulo_by_fraction - {{ 5 % 0.5 }} still panics
over-correct: divisor <= 0 in all three branches 1 int_modulo_negative_divisor - {{ 5 % -3 }} wrongly rejected

Verification

CI scripts copied out of the config files:

source command exit
.travis.yml script env GO111MODULE=on go test -v ./... 0
appveyor.yml test_script[0] go test -v ./... 0
extra go vet ./... 0
extra gofmt -l eval.go eval_test.go 0, nothing listed
extra go test -race on both zero-divisor tests 0
extra GOARCH=386 go build ./..., GOARCH=arm64 go build ./... 0

appveyor.yml test_script[1..4] (the examples/asset_packaging steps) need network for github.com/shurcooL/vfsgen, which is in neither go.mod nor go.sum, so I could not run them; they fail the same way on an unmodified checkout here and never compile eval.go. The Travis matrix does not run them at all.

Out of scope

The isFloat branch also saturates on conversion - {{ 1e19 % 10 }} prints -8, because the float-to-int64 conversion saturates. That is a separate and architecture-dependent question, this PR does not change it, and no test here asserts on it.


Disclosure: this patch was prepared with AI assistance. I ran the tests, the red-green, both mutation directions and the reachability probe above myself, and the numbers are from those runs.

`{{ a % b }}` with a zero divisor panicked with Go's `runtime error:
integer divide by zero`, and the panic escaped Template.Execute: recover()
in eval.go re-panics anything that is a runtime.Error, so an operator that
can produce one has to guard before it runs. itemDiv does that already;
itemMod did not. Modulo by zero now results in a runtime error instead of
a panic, like division by zero since f1947cd.

`%` has no float form, so the divisor is truncated by toInt/toUint before
it is used. `{{ 10 % 0.5 }}` therefore divides by zero as well, and the
guard sits on the converted divisor rather than on the raw right operand.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant