Hacker News new | ask | show | jobs
by DexesTTP 1488 days ago
I can actually comment on that fist question, having contributed to Serenity a bit in the past.

I did two big patches in the system, the first by adding websockets to the system[1][2] and the second (still ongoing) about allowing Browser to run in Linux directly[3]. Each went through several iterations trying to iron out all the lifetime and memory safety bugs I encountered, including in the first case one that got through the whole PR process and only got caught months afterwards.

The first change (WebSockets) took me a while to finalize, the main issue during its development was to solve the issues of the underlying socket never getting closed. This was actually due to RefCounted pointers getting implicitely captured by lambdas, which I solved by creating weak pointers for a bunch of them. This fix, however, ultimately broke the expected lifetime of the underlying socket by deleting it too early sometimes, which caused a sporadic disconnect issue. Now, this was not caught at all during the PR process, and was only found and fixed by Andreas a few months later[4]. This is directly a lifetime issue, and would have been caught by either a system that holds references directly.

The second change (headless-browser) took me at least twice as long than expected to do to make it work, due to some random crashes that I could not track down. This was ultimately diagnosed as me passing the wrong type of argument to the MemoryStream type, which accepted both BufferedSocket and TCP/TLSv1.2 Sockets directly but expected the behavior of BufferedSockets - it took two other people on Discord to debug this issue[5]. Note that that bug still allowed the headless browser to work for a little bit before it crashed, just not long enough that I didn't notice the bug. This bug was ultimately due to an invalid type constraint that I believe was left as-is as a workaround for another issue, but it could have been easily caught any language that didn't allow implicit reinterpretation of random memory data, so it ultimately is memory safety that lead to that happening.

Since I'm currently two for two in "memory safety-related issues" in my PRs, then yes I'd consider that, just like any other system, Serenity is having problems with memory safety - and that's just my personal experience. It's possible to do some things in C++ to try to limit these issues, and there's a lot of that being done in Serenity already. But in C++, all it takes is either a mistake by someone less skilled (me) or a workaround by someone who didn't think it'd be an issue, and the memory safety issues pop up again.

[1] https://github.com/SerenityOS/serenity/pull/6420

[2] https://github.com/SerenityOS/serenity/pull/6610

[3] https://github.com/SerenityOS/serenity/pull/13473

[4] https://github.com/SerenityOS/serenity/commit/1735d978ed2449...

[5] https://github.com/SerenityOS/serenity/pull/13473/commits/2d...

1 comments

Thank you. Detail is always welcome.

It looks as if the new language would not have helped the first problem, unless it lacks lambda support, or prevents capturing by copy. On first impression, I would chalk that fault up to over-use of refcounting, something that the new language seems like it would make worse.

The second seems like bypassing the type system, something you would have misused "unsafe" for in the new language.

I may have misunderstood something.