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

Sanitizer fixes and plProduct static init fix #1585

Merged
merged 4 commits into from
May 18, 2024
Merged

Conversation

dpogue
Copy link
Member

@dpogue dpogue commented Apr 27, 2024

  • plDetectorLog was initializing before plProduct had initialized its static strings, leading to some weird stuff where the User Data Folder path was missing "Uru Live"

  • Was running with AddressSanitizer to try to track down some GL Pipeline issues, and ended up tracking down a bunch of other issues and warnings first.

    • It is (apparently) undefined behaviour to cast non-memory-aligned values to int types, which we were doing in a few spots (and are still doing in a bunch more spots). This fixes the easy ones in plUUID and hsStream. Also, memcpy is a built-in now so there should be no speed benefit to doing casting.
    • fRegisteredForTime (and a few other values) were never being zero initialized in plTransitionMgr
    • plBufferedFileReader's fBuffer is created with new[] but was being released with delete

Sources/Plasma/NucleusLib/pnUUID/pnUUID_Unix.cpp Outdated Show resolved Hide resolved
Sources/Plasma/NucleusLib/pnUUID/pnUUID_Unix.cpp Outdated Show resolved Hide resolved
Sources/Plasma/NucleusLib/pnUUID/pnUUID_Unix.cpp Outdated Show resolved Hide resolved
Sources/Plasma/NucleusLib/pnUUID/pnUUID_Unix.cpp Outdated Show resolved Hide resolved
@colincornaby
Copy link
Contributor

Thanks - the address sanitizer stuff was bugging me for a while. Hopefully this is the same issues I was seeing.

@dpogue dpogue force-pushed the san-fixes branch 2 times, most recently from 56c4a36 to cfd9402 Compare April 29, 2024 02:42
@dpogue dpogue changed the title Sanitizer fixes and plDetectorLog static init fix Sanitizer fixes and plProduct static init fix Apr 29, 2024
@dpogue
Copy link
Member Author

dpogue commented Apr 29, 2024

I have dropped the change to plDetectorLog and instead made more of the plProduct strings use std::string_view.

Copy link
Contributor

@dgelessus dgelessus left a comment

Choose a reason for hiding this comment

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

The code changes look good to me overall now. (I have no particular opinions on the things @Hoikas commented)

cmake/hsBuildInfo.inl.cmake Outdated Show resolved Hide resolved
@dpogue dpogue marked this pull request as draft April 30, 2024 00:36
@dpogue dpogue marked this pull request as ready for review April 30, 2024 05:07
@Hoikas
Copy link
Member

Hoikas commented May 1, 2024

Looks like we have a test failing on macOS.

@dpogue
Copy link
Member Author

dpogue commented May 3, 2024

Looks like we have a test failing on macOS.

The pnUUID test was failing on the string comparison due to case differences. I've worked around that for now by using EXPECT_STRCASEEQ but I wanted to raise it for discussion.

It seems that Linux and Windows stringify plUUID with lowercase letters. On macOS it's using uppercase letters. I suspect the plUUID AsString() is only used for debugging, but do we want to enforce lowercase consistency across all OSes?

This should avoid some static initialization ordering issues that were
cropping up with ST::string construction.
@dpogue
Copy link
Member Author

dpogue commented May 4, 2024

Okay, this should be good to go now. I opted to enforce that stringified UUIDs are lowercase for consistency with existing Windows code.

@colincornaby
Copy link
Contributor

colincornaby commented May 4, 2024

I'll toss in that just for "technically correct"-ness - UUIDs should always be compared case insensitively.

https://datatracker.ietf.org/doc/html/rfc4122

The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.

Apple's implementation looks out of line from the spec (I'd be curious as to why) as they output in upper case - but not in a way that should actually impact UUID use. Something to keep in mind for any sort of comparisons we do with UUIDs though.

(Backtracking through the UUID specifications is kind of weird in since their use was spread so wide, by my understanding is RFC4122 restates the UUID specification.)

@dpogue
Copy link
Member Author

dpogue commented May 4, 2024

Something to keep in mind for any sort of comparisons we do with UUIDs though.

Any comparisons of UUIDs should be done bitwise by the plUUID class, this only came to light because I added a unit test that checked the stringified output against a known value.

@dpogue dpogue merged commit fba88f2 into H-uru:master May 18, 2024
17 checks passed
@dpogue dpogue deleted the san-fixes branch May 18, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants