Skip to content

Stack Corruption [Compiler Bug] #22514

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

Closed
Reaper1121 opened this issue Jan 17, 2025 · 3 comments
Closed

Stack Corruption [Compiler Bug] #22514

Reaper1121 opened this issue Jan 17, 2025 · 3 comments
Labels
question No questions on the issue tracker, please.

Comments

@Reaper1121
Copy link

Reaper1121 commented Jan 17, 2025

Zig Version

0.14.0-dev.2649+77273103a

Steps to Reproduce and Observed Behavior

I've been working on my first Zig project and while beginning work on my own standard library for Zig, during testing I've ran into segmentation faults during UUID comparison that shouldn't occur.

I initially thought the issue is coming from "std.mem.eql" function and quickly wrote my own "memcmp" which didn't help. When looking into it deeper at assembly level, I couldn't quite figure out what's exactly happening but I noticed that the UUID's passed by the stack are being corrupted via the "call" instruction, the return pointer push to the stack. I've observed memory corruption happen with other methods of comparing the UUID, such as by reference (pointer).

Bug repo:
I have pushed the code of the redacted project to a repo with the other UUID comparison methods that can be used to replicate the issue: https://github.com/Reaper1121/zig-compiler-bug

About the bug:
The bug occurs in the "deps/zstdplus/oop/Object.zig" file at line 51, put a breakpoint on that line and observe the stack corruption happen at assembly level, at the "call" instruction to "UUID.compare_vals" method.
I've tested different versions of Zig, that is 0.13.0, 0.14.0-dev.2649+77273103a and the latest commit as of 01-16 month/day.
I haven't tested any other platforms but Linux on x86_64 architecture.
The bug appears to happen only within "Debug" optimization mode/build, the "Release mode does not appear to trigger the compiler bug, though I haven't done thorough testing of it in Release mode.

Reproduce:

Just run "zig build run" of the redacted project and it will result into a segmentation fault, other UUID comparison methods will lead to non-fatal memory corruption that can be seen by looking at the object headers.

Expected Behavior

The application shouldn't segfault, all of the memory pointers are valid.

@Reaper1121 Reaper1121 added the bug Observed behavior contradicts documented or intended behavior label Jan 17, 2025
@mlugg
Copy link
Member

mlugg commented Jan 18, 2025

A quick check with a debugger confirms that there is no stack corruption going on. In the FindType call, Arg_This.NextHeaderPtr.?.NextHeaderPtr is just an invalid pointer (when I tried, it was 0x1f).

I haven't looked into this too deeply, but your codebase exhibits Unchecked Illegal Behavior, as you use @ptrCast to convert between a pointer to TestObject and a pointer to its first field. If you need guarantees about memory layout, use an extern struct. This is almost certainly the cause of your issues.

By the way, your code is extremely atypical Zig, and users of the language will more than likely find it difficult to understand your code. The lack of OOP constructs in the language isn't a suggestion to reinvent them all in userland!

I'd enourage you to join a Zig community, both to understand the bugs in your code and to get stylistic / paradigm suggestions.

@mlugg mlugg closed this as not planned Won't fix, can't repro, duplicate, stale Jan 18, 2025
@mlugg mlugg added question No questions on the issue tracker, please. and removed bug Observed behavior contradicts documented or intended behavior labels Jan 18, 2025
@Reaper1121
Copy link
Author

Reaper1121 commented Jan 18, 2025

@mlugg Instead of assuming that it's a user error from a person with 20 years of experience in low level programming, even though I am new to the Zig programming language I'd suggest trying to address the issue step by step, cooperating with the user.

Zig is a general purpose programming language and should not restrict the user from how the user may want to use the language, C is used as a foundation for many ridiculous things, and just to stab you back I'll remind you that Zig uses interfaces in the standard library excessively, such as the writer, allocator, etc... which is a OOP concept, from the right perspective nearly everything in Zig is a object, even imports unless "usingnamespace" is used which is much closer to C/C++ nature.

Though I am not here to argue or fight but to address a severe bug with the compiler and this is my last attempt to raise awareness to the problem, otherwise I am going back to the C/C++ world.

I have cleaned up the bug report repo code with heavy commenting, I have observed the stack corruption and verified everything many times. I do agree that I shouldn't have posted bug report code in the terrible state that it was, I doubt anyone would want to deal with that mess.

Please, take a look at this bug report one more time as I've made multiple stack corruptions easily observable for you through GDB, though I don't intend to write enterprise quality code for a bug report, especially when the bug is so fragile, every minor code or even file movements/changes causes behavior shift.

The Zig compiler version I've tested this on is the same as in the original report.
I have focused on the "Debug" build for this bug report, as "Release" build's alter the behavior of the stack corruption bug drastically.

@mlugg
Copy link
Member

mlugg commented Jan 18, 2025

Your updated code has a stack UAF bug in NewStackCorruption, caused by storing the pointer &Func_TestObject._Header (which points to a stack local) into Func_TestObject which is then returned. I'm not sure if this issue was also there before, or if your attempt to reduce your issue changed it; regardless, this stack UAF triggers Unchecked Illegal Behavior. Related: #2646

Instead of assuming that it's a user error from a person with 20 years of experience in low level programming...

I did not "assume it is user error": I read your code, saw some bugs, and told you about them.

[...]

...and just to stab you back...

This is quite immature behavior for an issue tracker; I didn't "stab you". I explained your issue, made good-faith suggestions, and directed you to a place you could learn more.

...unless "usingnamespace" is used...

In case you're not aware, note that usingnamespace may be removed from the language soon; see #20663.


If you have further questions, please join a community.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question No questions on the issue tracker, please.
Projects
None yet
Development

No branches or pull requests

2 participants