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.

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

Running bun in valgrind shows an excessive amount of memory safety issues (false positives maybe?) #579

Open
FSMaxB opened this issue Jul 11, 2022 · 4 comments
Labels
performance An issue with performance

Comments

@FSMaxB
Copy link

FSMaxB commented Jul 11, 2022

Version

0.1.2

Platform

Linux <my_hostname> 5.18.10-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 07 Jul 2022 17:18:13 +0000 x86_64 GNU/Linux

What steps will reproduce the bug?

Using valgrind in version 3.19.0, run valgrind bun ./http.ts with:

export default {
        port: 3000,
        fetch(fequest: Request) {
                return new Response("Hello World");
        },
};

Wait for a few seconds and it starts printing hundreds of detected issues. At this point you can stop it with Ctrl-C.

How often does it reproduce? Is there a required condition?

Works quite reliably.

What is the expected behavior?

No memory safety issues reported by valgrind.

What do you see instead?

Hundres of reported issues.

log.txt

Additional information

I'm not sure if those are real issues or if those are actually safe operations that valgrind just can't track properly because it wasn't designed for programs written with zig.

If they are real issues though, I'd call that a bit concerning.

@FSMaxB FSMaxB added bug Something isn't working needs repro Needs an example to reproduce labels Jul 11, 2022
@FSMaxB
Copy link
Author

FSMaxB commented Jul 11, 2022

Could those be false positives due to JIT compilation perhaps?

@FSMaxB FSMaxB changed the title Running bun in valgrind shows an excessive amount of memory safety issues Running bun in valgrind shows an excessive amount of memory safety issues (false positives maybe?) Jul 11, 2022
@Jarred-Sumner
Copy link
Collaborator

Jarred-Sumner commented Jul 11, 2022

in debug builds I tend to use mimalloc's debug mode which adds some additional safety checks, but I also haven't run Bun through sanitizers yet. It's worth investigating these.

The first one from the log:

==68479== Use of uninitialised value of size 8
==68479== at 0x37C3383: JSClassRetain (in /usr/lib/bun/bun)
==68479== by 0x37B4926: ??? (in /usr/lib/bun/bun)
==68479== by 0x4BB49BD: ??? (in /usr/lib/bun/bun)
==68479== by 0x4BB3F67: ??? (in /usr/lib/bun/bun)
==68479== by 0x3E32478: ??? (in /usr/lib/bun/bun)
==68479== by 0x3E0F877: ??? (in /usr/lib/bun/bun)
==68479== by 0x3E58476: ??? (in /usr/lib/bun/bun)
==68479== by 0x3E16784: ??? (in /usr/lib/bun/bun)
==68479== by 0x3E1092F: main (in /usr/lib/bun/bun)

The only call to JSClassRetain in bun is here:

https://github.com/Jarred-Sumner/bun/blob/main/src/bun.js/base.zig#L958-L959

We can see that lazy_ref is a threadlocal variable defined here and it does not use undefined memory:

https://github.com/Jarred-Sumner/bun/blob/main/src/bun.js/base.zig#L908-L909

Bun does use a lot of threadlocal variables, many of which are defined later

image

Will continue investigating

@Jarred-Sumner Jarred-Sumner removed bug Something isn't working needs repro Needs an example to reproduce labels Jul 11, 2022
@Electroid Electroid added the performance An issue with performance label Nov 2, 2022
@guest271314

This comment was marked as off-topic.

@FSMaxB
Copy link
Author

FSMaxB commented May 22, 2023

Bun does use a lot of threadlocal variables, many of which are defined later

Are you saying that threadlocal variables are falsely flagged as uninitialized read by valgrind?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance An issue with performance
Projects
None yet
Development

No branches or pull requests

4 participants