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

Fix: std.json: Writing inf values returns InvalidJSON error #23258

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+42 −6
Open

Fix: std.json: Writing inf values returns InvalidJSON error #23258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
516e7b3
std.json: Writing inf values returns InvalidJSON error
bradcypert Mar 15, 2025
fa745c8
use std.math.isInf and std.math.isNan
bradcypert Mar 15, 2025
935b028
update error to match zig style
bradcypert Mar 15, 2025
d107983
include InvalidJson in error return in both form of safety checks
bradcypert Mar 16, 2025
a8be9cd
Update error, split out comptime float handling
bradcypert Mar 18, 2025
3c83b3c
Update other stringify return types
bradcypert Mar 18, 2025
7a4724e
Merge branch 'master' into brad/23253-json-infinity-fix
bradcypert Mar 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions lib/std/json/stringify.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ pub fn stringify(
value: anytype,
options: StringifyOptions,
out_stream: anytype,
) @TypeOf(out_stream).Error!void {
) WriteStream(@TypeOf(out_stream), .checked_to_arbitrary_depth).Error!void {
var jw = writeStream(out_stream, options);
defer jw.deinit();
try jw.write(value);
Expand All @@ -61,7 +61,7 @@ pub fn stringifyMaxDepth(
options: StringifyOptions,
out_stream: anytype,
comptime max_depth: ?usize,
) @TypeOf(out_stream).Error!void {
) WriteStream(@TypeOf(out_stream), .checked_to_arbitrary_depth).Error!void {
var jw = writeStreamMaxDepth(out_stream, options, max_depth);
try jw.write(value);
}
Expand All @@ -88,7 +88,7 @@ pub fn stringifyAlloc(
allocator: Allocator,
value: anytype,
options: StringifyOptions,
) error{OutOfMemory}![]u8 {
) error{ OutOfMemory, InvalidInput }![]u8 {
var list = std.ArrayList(u8).init(allocator);
errdefer list.deinit();
try stringifyArbitraryDepth(allocator, value, options, list.writer());
Expand Down Expand Up @@ -195,8 +195,8 @@ pub fn WriteStream(

pub const Stream = OutStream;
pub const Error = switch (safety_checks) {
.checked_to_arbitrary_depth => Stream.Error || error{OutOfMemory},
.checked_to_fixed_depth, .assumed_correct => Stream.Error,
.checked_to_arbitrary_depth => Stream.Error || error{ OutOfMemory, InvalidInput },
.checked_to_fixed_depth, .assumed_correct => Stream.Error || error{InvalidInput},
};

options: StringifyOptions,
Expand Down Expand Up @@ -468,6 +468,7 @@ pub fn WriteStream(
/// * Zig `i32`, `u64`, etc. -> JSON number or string.
/// * When option `emit_nonportable_numbers_as_strings` is true, if the value is outside the range `+-1<<53` (the precise integer range of f64), it is rendered as a JSON string in base 10. Otherwise, it is rendered as JSON number.
/// * Zig floats -> JSON number or string.
/// * If the value is NaN or infinity, an error is returned.
/// * If the value cannot be precisely represented by an f64, it is rendered as a JSON string. Otherwise, it is rendered as JSON number.
/// * TODO: Float rendering will likely change in the future, e.g. to remove the unnecessary "e+00".
/// * Zig `[]const u8`, `[]u8`, `*[N]u8`, `@Vector(N, u8)`, and similar -> JSON string.
Expand Down Expand Up @@ -509,7 +510,22 @@ pub fn WriteStream(
.comptime_int => {
return self.write(@as(std.math.IntFittingRange(value, value), value));
},
.float, .comptime_float => {
.comptime_float => {
if (@as(f64, @floatCast(value)) == value) {
try self.valueStart();
try self.stream.print("{}", .{@as(f64, @floatCast(value))});
self.valueDone();
return;
}
try self.valueStart();
try self.stream.print("\"{}\"", .{value});
self.valueDone();
return;
},
.float => {
if (std.math.isInf(value) or std.math.isNan(value)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure, but I think you can combine the .float and .comptime_float cases back together if you do the f64 cast first (which also makes a comptime type a runtime type) and use that as the argument to isInf/isNan:

const f64val: f64 = @floatCast(value);
if (std.math.isInf(f64val) or std.math.isNan(f64val)) {
   return error.InvalidInput;
}
if (f64val == value) {
...

(I think you should also use the fval in the first print too.... You might even refactor this code a bit and move the valueStart/valueDone such that they're done once? Not really your change though.)

Also, I'm not sure ... but is the fval == value check redundant with the isNan check? Ah, no... this is for detecting floats that don't fit in an f64 and rendering them as strings.

return error.InvalidInput;
}
if (@as(f64, @floatCast(value)) == value) {
try self.valueStart();
try self.stream.print("{}", .{@as(f64, @floatCast(value))});
Expand Down
20 changes: 20 additions & 0 deletions lib/std/json/stringify_test.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,26 @@ test "nonportable numbers" {
try testStringify("\"9999999999999999\"", 9999999999999999, .{ .emit_nonportable_numbers_as_strings = true });
}

test "invalid JSON values" {
try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, std.math.inf(f32), .{}));
try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, std.math.inf(f64), .{}));
try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, std.math.nan(f32), .{}));
try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, std.math.nan(f64), .{}));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think its worth adding an f128 test case too here to make sure the doesn't-fit-in-an-f64 path is handled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like there is test coverage of the does-not-fit-in-f64 path elsewhere, so for bonus points you could add some success-path coverage of that too.


try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, .{
.a = std.math.inf(f32),
}, .{}));
try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, .{
.a = std.math.inf(f64),
}, .{}));
try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, .{
.a = std.math.nan(f32),
}, .{}));
try std.testing.expectError(error.InvalidInput, stringifyAlloc(std.testing.allocator, .{
.a = std.math.nan(f64),
}, .{}));
}

test "stringify raw streaming" {
var out_buf: [1024]u8 = undefined;
var slice_stream = std.io.fixedBufferStream(&out_buf);
Expand Down