Skip to content

Comments

mem: fix UB in readInt/writeInt and delete variants#17802

Merged
andrewrk merged 5 commits intoziglang:masterfrom
jacobly0:read-write-int
Nov 1, 2023
Merged

mem: fix UB in readInt/writeInt and delete variants#17802
andrewrk merged 5 commits intoziglang:masterfrom
jacobly0:read-write-int

Conversation

@jacobly0
Copy link
Member

@jacobly0 jacobly0 commented Oct 31, 2023 •
edited
Loading

In Zig, loads and stores affect @sizeOf(T) bytes of memory. The old implementations of these functions were pointer casting a buffer of @divExact(@bitSizeOf(T), 8) bytes which is not guaranteed to be large enough to perform a load or store of T directly. This only ever worked because llvm has weird store size rules, and in fact, after this change, some non-llvm backends mysteriously started passing these tests.

By making use of inline, the complicated nest of variant functions could all be reduced to four functions without compromising functionality. After removing some incorrect uses of two of these, there were no references to them remaining, so I deleted them too.

Migration guide

These apply to both the functions in std.mem and the methods on std.io.GenericReader, std.io.AnyReader, and std.io.Writer.

  • readInt(...) → readInt(...)
  • readIntLittle(...) → readInt(..., .little)
  • readIntBig(...) → std.mem.readInt(..., .big)
  • readIntNative(...) → std.mem.readInt(..., @import("builtin").cpu.arch.endian()) or just @as(T, @bitCast(buffer.*)) for the std.mem version
  • readIntForeign(...) → @byteSwap(std.mem.readInt(..., @import("builtin").cpu.arch.endian())) or just @byteSwap(@as(T, @bitCast(buffer.*))) for the std.mem version
  • writeInt(...) → std.mem.writeInt(...)
  • writeIntLittle(...) → std.mem.writeInt(..., .little)
  • writeIntBig(...) → std.mem.writeInt(..., .big)
  • writeIntNative(...) → std.mem.writeInt(..., @import("builtin").cpu.arch.endian()) or just buffer.* = @bitCast(value) for the std.mem version
  • writeIntForeign(...) → std.mem.writeInt(..., @byteSwap(value), @import("builtin").cpu.arch.endian()) or just buffer.* = @bitCast(@byteSwap(value)) for the std.mem version

@Luukdegram unfortunately, this new pattern regressed the self-hosted wasm backend, so just letting you know that there are three new .stage2_wasm checks in lib/test_runner.zig, test/behavior/bugs/1851.zig, and test/behavior/struct.zig.

@jacobly0 jacobly0 requested a review from squeek502 as a code owner October 31, 2023 10:31
@jacobly0 jacobly0 mentioned this pull request Oct 31, 2023
Merged
@andrewrk andrewrk added breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library. labels Oct 31, 2023
@andrewrk andrewrk enabled auto-merge October 31, 2023 17:29
@andrewrk andrewrk disabled auto-merge October 31, 2023 19:17
@andrewrk andrewrk enabled auto-merge October 31, 2023 20:04
@andrewrk
Copy link
Member

andrewrk commented Oct 31, 2023

These warnings are new when bootstrapping:

[10/13] Building C object CMakeFiles/zig2.dir/zig2.c.o
/home/andy/Downloads/zig/build-release/zig2.c: In function ‘value_Value_readFromMemory__22826’:
/home/andy/Downloads/zig/build-release/zig2.c:1503723: warning: ‘memcpy’ reading 16 bytes from a region of size 10 [-Wstringop-overread]
1503723 |      memcpy(&t47, &t46, sizeof(zig_u128));
        | 
/home/andy/Downloads/zig/build-release/zig2.c:1503582: note: source object ‘t46’ of size 10
1503582 |  uint8_t t46[10];
        | 
/home/andy/Downloads/zig/build-release/zig2.c: In function ‘mem_readPackedIntLittle__anon_233795__233795’:
/home/andy/Downloads/zig/build-release/zig2.c:2732095: warning: ‘memcpy’ reading 16 bytes from a region of size 10 [-Wstringop-overread]
2732095 |  memcpy(&t11, &t10, sizeof(zig_u128));
        | 
/home/andy/Downloads/zig/build-release/zig2.c:2732072: note: source object ‘t10’ of size 10
2732072 |  uint8_t t10[10];
        | 
/home/andy/Downloads/zig/build-release/zig2.c: In function ‘mem_readPackedIntBig__anon_233796__233796’:
/home/andy/Downloads/zig/build-release/zig2.c:2732169: warning: ‘memcpy’ reading 16 bytes from a region of size 10 [-Wstringop-overread]
2732169 |  memcpy(&t13, &t12, sizeof(zig_u128));
        | 
/home/andy/Downloads/zig/build-release/zig2.c:2732131: note: source object ‘t12’ of size 10
2732131 |  uint8_t t12[10];
        | 
[11/13] Building stage3

squeek502
squeek502 approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@squeek502 squeek502 left a comment

Choose a reason for hiding this comment

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

Changes to resinator look correct.

@andrewrk
Copy link
Member

andrewrk commented Nov 1, 2023 

D:\a\zig\zig\lib\zig.h(2183): warning C4244: 'function': conversion from 'uint16_t' to 'uint8_t', possible loss of data
...
D:\a\zig\zig\lib\zig.h(3366): warning C4391: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect return type for intrinsic function, expected 'double'
D:\a\zig\zig\lib\zig.h(3366): warning C4392: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect number of arguments for intrinsic function, expected '2' arguments
test-x86_64-windows-msvc.c(53486): warning C4133: 'function': incompatible types - from 'A__9111 *' to 'A__9107 *'
test-x86_64-windows-msvc.c(53488): warning C4133: 'function': incompatible types - from 'A__9112 *' to 'A__9107 *'
test-x86_64-windows-msvc.c(65402): warning C4047: '=': 'void (__cdecl *)(void)' differs in levels of indirection from 'uint8_t (*)[4]'
test-x86_64-windows-msvc.c(123940): error C2167: '__fmax': too many actual parameters for intrinsic function
test-x86_64-windows-msvc.c(124357): error C2167: '__fmax': too many actual parameters for intrinsic function
test-x86_64-windows-msvc.c(263186): warning C4090: '=': different 'const' qualifiers
D:\a\zig\zig\lib\zig.h(2183): warning C4244: 'function': conversion from 'uint16_t' to 'uint8_t', possible loss of data
...
D:\a\zig\zig\lib\zig.h(3366): warning C4391: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect return type for intrinsic function, expected 'double'
D:\a\zig\zig\lib\zig.h(3366): warning C4392: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect number of arguments for intrinsic function, expected '2' arguments
compiler_rt-x86_64-windows-msvc.c(490): warning C4392: 'zig_f80 __fmax(zig_f80,zig_f80,zig_f80)': incorrect number of arguments for intrinsic function, expected '2' arguments
compiler_rt-x86_64-windows-msvc.c(19292): warning C4392: 'zig_f80 __fmax(const zig_f80,const zig_f80,const zig_f80)': incorrect number of arguments for intrinsic function, expected '2' arguments
compiler_rt-x86_64-windows-msvc.c(19292): error C2169: '__fmax': intrinsic function, cannot be defined

huh. I don't understand how that passed the CI before, or why it's passing on x86_64-windows-debug.

@jacobly0
Copy link
Member Author

jacobly0 commented Nov 1, 2023 •
edited
Loading

These warnings are new when bootstrapping:

Have a fix, but waiting for a CI run to complete.

huh. I don't understand how that passed the CI before, or why it's passing on x86_64-windows-debug.

It's obvious what happened (clash between f-max and fma-x), but like you said, who knows why it happened on this run in particular. I can only assume either a bad roll gave us a different image (different compiler version?) somehow, or an msvc bugs means the error is not consistent.

@jacobly0
Copy link
Member Author

jacobly0 commented Nov 1, 2023

Well, not related to the changes in this PR at least: https://github.com/ziglang/zig/actions/runs/6710563026/job/18236031654

jacobly0 and others added 5 commits October 31, 2023 21:37
@jacobly0
Sema: fix incorrect error comptime-mutating empty array
50cdb65
@jacobly0
mem: fix ub in writeInt
d890e81 
Use inline to vastly simplify the exposed API.  This allows a
comptime-known endian parameter to be propogated, making extra functions
for a specific endianness completely unnecessary.
@jacobly0
mem: delete readIntSlice and writeIntSlice
149200a 
After deleting all potentially incorrect usages, there were no more
remaining.
@andrewrk @jacobly0
std.builtin.Endian: make the tags lower case
3fc6fc6 
Let's take this breaking change opportunity to fix the style of this
enum.
@jacobly0
cbe: fix @bitCast warnings
13b1e10
@andrewrk andrewrk merged commit 725f765 into ziglang:master Nov 1, 2023
@jacobly0 jacobly0 deleted the read-write-int branch November 1, 2023 10:48
@Luukdegram Luukdegram mentioned this pull request Nov 1, 2023
Merged
@BratishkaErik BratishkaErik mentioned this pull request Nov 2, 2023
Closed
@jcalabro jcalabro mentioned this pull request Nov 5, 2023
Merged
scheibo added a commit to pkmn/engine that referenced this pull request Nov 6, 2023
@scheibo
fix writeIntNative calls post ziglang/zig#17802
4f7cbb8
@jhmaster2000 jhmaster2000 mentioned this pull request Nov 17, 2023
Merged
@BratishkaErik BratishkaErik mentioned this pull request Nov 18, 2023
Closed
@BratishkaErik BratishkaErik mentioned this pull request Dec 22, 2023
Closed
@jacobly0 jacobly0 mentioned this pull request Apr 24, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@squeek502 squeek502 squeek502 approved these changes

Assignees

No one assigned

Labels

breaking Implementing this issue could cause existing code to no longer compile or have different behavior. standard library This issue involves writing Zig code for the standard library.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jacobly0 @andrewrk @squeek502