Skip to content

Commit

Permalink
fix: make sure we can parse integers from JSON (ooni#1435)
Browse files Browse the repository at this point in the history
This commit ensures we can parse integer values provided using OONI Run
v2 descriptors into actual integers.

Values are originally parsed as float64, so we need to add a specific
conversion case for that scenario.

Diff extracted from: ooni#1423

Closes ooni/probe#2645
  • Loading branch information
bassosimone authored and Murphy-OrangeMud committed Feb 13, 2024
1 parent bf7a9df commit b3c293d
Show file tree
Hide file tree
Showing 3 changed files with 64 additions and 0 deletions.
2 changes: 2 additions & 0 deletions internal/cmd/miniooni/oonirun.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ func ooniRunMain(ctx context.Context,
logger.Warnf("oonirun: parsing OONI Run v2 descriptor failed: %s", err.Error())
continue
}
logger.Infof("oonirun: running '%s'", descr.Name)
logger.Infof("oonirun: link authored by '%s'", descr.Author)
if err := oonirun.V2MeasureDescriptor(ctx, cfg, &descr); err != nil {
logger.Warnf("oonirun: running link failed: %s", err.Error())
continue
Expand Down
19 changes: 19 additions & 0 deletions internal/registry/factory.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package registry
import (
"errors"
"fmt"
"math"
"os"
"reflect"
"strconv"
Expand Down Expand Up @@ -107,6 +108,12 @@ func (b *Factory) setOptionBool(field reflect.Value, value any) error {
}
}

// With JSON we're limited by the 52 bits in the mantissa
const (
jsonMaxInteger = 1<<53 - 1
jsonMinInteger = -1<<53 + 1
)

// setOptionInt sets an int option
func (b *Factory) setOptionInt(field reflect.Value, value any) error {
switch v := value.(type) {
Expand All @@ -132,6 +139,18 @@ func (b *Factory) setOptionInt(field reflect.Value, value any) error {
}
field.SetInt(number)
return nil
case float64:
if math.IsNaN(v) || math.IsInf(v, 0) {
return fmt.Errorf("%w from: %v", ErrCannotSetIntegerOption, value)
}
if math.Trunc(v) != v {
return fmt.Errorf("%w from: %v", ErrCannotSetIntegerOption, value)
}
if v > jsonMaxInteger || v < jsonMinInteger {
return fmt.Errorf("%w from: %v", ErrCannotSetIntegerOption, value)
}
field.SetInt(int64(v))
return nil
default:
return fmt.Errorf("%w from a value of type %T", ErrCannotSetIntegerOption, value)
}
Expand Down
43 changes: 43 additions & 0 deletions internal/registry/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package registry
import (
"errors"
"fmt"
"math"
"os"
"testing"

Expand Down Expand Up @@ -251,6 +252,48 @@ func TestExperimentBuilderSetOptionAny(t *testing.T) {
FieldValue: make(chan any),
ExpectErr: ErrCannotSetIntegerOption,
ExpectConfig: &fakeExperimentConfig{},
}, {
TestCaseName: "[int] for NaN",
InitialConfig: &fakeExperimentConfig{},
FieldName: "Value",
FieldValue: math.NaN(),
ExpectErr: ErrCannotSetIntegerOption,
ExpectConfig: &fakeExperimentConfig{},
}, {
TestCaseName: "[int] for +Inf",
InitialConfig: &fakeExperimentConfig{},
FieldName: "Value",
FieldValue: math.Inf(1),
ExpectErr: ErrCannotSetIntegerOption,
ExpectConfig: &fakeExperimentConfig{},
}, {
TestCaseName: "[int] for -Inf",
InitialConfig: &fakeExperimentConfig{},
FieldName: "Value",
FieldValue: math.Inf(-1),
ExpectErr: ErrCannotSetIntegerOption,
ExpectConfig: &fakeExperimentConfig{},
}, {
TestCaseName: "[int] for too large value",
InitialConfig: &fakeExperimentConfig{},
FieldName: "Value",
FieldValue: float64(jsonMaxInteger + 1),
ExpectErr: ErrCannotSetIntegerOption,
ExpectConfig: &fakeExperimentConfig{},
}, {
TestCaseName: "[int] for too small value",
InitialConfig: &fakeExperimentConfig{},
FieldName: "Value",
FieldValue: float64(jsonMinInteger - 1),
ExpectErr: ErrCannotSetIntegerOption,
ExpectConfig: &fakeExperimentConfig{},
}, {
TestCaseName: "[int] for float64 with nonzero fractional value",
InitialConfig: &fakeExperimentConfig{},
FieldName: "Value",
FieldValue: 1.11,
ExpectErr: ErrCannotSetIntegerOption,
ExpectConfig: &fakeExperimentConfig{},
}, {
TestCaseName: "[string] for serialized bool value while setting a string value",
InitialConfig: &fakeExperimentConfig{},
Expand Down

0 comments on commit b3c293d

Please sign in to comment.