In rP19674 depending on the linking order tempCacheFileName and cacheFileName could be constructed using the uninitialized g_args in GetDirectoryName and thereby segfault. This patch fixes that. It also renames GetDirectoryName to GetDirectoryPath because it returns a path and not a name.
Details
- Reviewers
weberkai vladislavbelov - Commits
- rP22284: Fix possibly using uninitialized global in rP19674 and rename `GetDirectoryName`
- Trac Tickets
- #4789
For most the behaviour shouldn't change. For slackware it shouldn't segfault.
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Source... source/ps/scripting/JSInterface_VisualReplay.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" source/ps/VisualReplay.h | 1| /*·Copyright·(C)·2017·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2017" source/ps/Replay.cpp | 1| /*·Copyright·(C)·2018·Wildfire·Games. | | [NORMAL] LicenseYearBear: | | License should have "2019" year instead of "2018" Executing section JS... Executing section cli...
Link to build: https://jenkins.wildfiregames.com/job/differential/1305/display/redirect
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1306/display/redirect
Hi!
Good news!
The Differential D1852 fix the issue!
Awesome work guys! Congratulations!
Thank you very much!
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1310/display/redirect
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | Why is it static const variable? Just return directly? |
source/ps/VisualReplay.h | ||
35 ↗ | (On Diff #7882) | -scriptInterface |
48 ↗ | (On Diff #7882) | 14 years of merkel were pretty bad (ambiguous description) @return lines consist of the same information of the line above and below it, so IMO unnecessary lines |
source/ps/VisualReplay.cpp | ||
---|---|---|
516 ↗ | (On Diff #7882) | Noticed that too sometime after February 15th in my personal trash branch: -JS::Value VisualReplay::GetReplayMetadata(ScriptInterface::CxPrivate* pCxPrivate, const OsPath& directoryName) +JS::Value VisualReplay::GetReplayMetadata(ScriptInterface::CxPrivate* pCxPrivate, const VfsPath& replayDirectory) { - if (!HasReplayMetadata(directoryName)) - return JSVAL_NULL; + if (!HasReplayMetadata(replayDirectory)) + return JS::UndefinedValue(); JSContext* cx = pCxPrivate->pScriptInterface->GetContext(); JSAutoRequest rq(cx); - JS::RootedValue metadata(cx); - - std::ifstream stream(OsString(GetDirectoryName() / directoryName / L"metadata.json").c_str()); - ENSURE(stream.good()); - CStr line; - std::getline(stream, line); - pCxPrivate->pScriptInterface->ParseJSON(line, &metadata); - - return metadata; + JS::RootedValue out(cx); + pCxPrivate->pScriptInterface->ReadJSONFile(VfsPath(L"replays") / replayDirectory / VfsPath(L"metadata.json"), &out); + return out; } |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | Why should it only be constructed once? |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1368/display/redirect
direct return
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | I thought caching would be more efficient. |
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/1369/display/redirect
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | g_args is not constant, so if it changes, then GetDirectoryPath returns a wrong value, which is even the case in the current code if it doesn't segfault: (Hypothetically UserData() may also differ.) |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | When does g_args change? afaics it doesn't. Previous code didn't cache g_args, so the current diff doesn't change behaviour. |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | In the current code (i.e. prior to this diff), on platforms other than slackware, as mentioned, g_args is first default constructed in ps/Mod.cpp:CmdLineArgs g_args;, then static const OsPath cacheFileName = VisualReplay::GetDirectoryName() / L"replayCache.json"; is resolved and set into stone (const), and then main.cpp: g_args = args;. So the previous code would even be wrong even if it was not undefined behavior with regards to the init order, and even if g_args never changes after the call in main.cpp: g_args = args; g_args is not marked as constant, so by definition it has the capacity to change in the future even if there is no other assignment to it yet. |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | True. |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | Why make the function return the wrong value in a more performant way? |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | Huh? |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | Perfect. So the current patch should be fine for you? |
source/ps/VisualReplay.cpp | ||
---|---|---|
53 ↗ | (On Diff #7882) | The currently committed code is fine for me too because I'm not using slackware. |