Page MenuHomeWildfire Games

Fix possibly using uninitialized global in rP19674 and rename `GetDirectoryName`
ClosedPublic

Authored by Imarok on Apr 28 2019, 4:50 PM.

Details

Summary

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.

Test Plan

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

Imarok created this revision.Apr 28 2019, 4:50 PM

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

Imarok updated this revision to Diff 7882.Apr 28 2019, 4:57 PM

Fix years

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!

weberkai accepted this revision.Apr 29 2019, 6:00 AM
This revision is now accepted and ready to land.Apr 29 2019, 6:00 AM
source/ps/VisualReplay.cpp
46 ↗(On Diff #7882)

Maybe we need to cache it too? Can the g_args be changed after the first initialisation?

516 ↗(On Diff #7882)

Strange lines here and above, why it wasn't just std::ifstream stream(...).

Imarok updated this revision to Diff 7887.May 1 2019, 12:31 AM

Cache replay directory path

Imarok marked an inline comment as done.May 1 2019, 12:31 AM
Imarok added inline comments.
source/ps/VisualReplay.cpp
46 ↗(On Diff #7882)

True, seems like a valid proposal.

516 ↗(On Diff #7882)

No idea. Guess you'd have to ask elexis. ;)

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1310/display/redirect

elexis removed a reviewer: elexis.May 2 2019, 9:59 PM
elexis added inline comments.
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
-return (see comment below)

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

elexis added a subscriber: elexis.May 2 2019, 10:06 PM
elexis added inline comments.
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;
 }
Imarok updated this revision to Diff 7974.May 11 2019, 10:56 PM
Imarok marked 3 inline comments as done.

Fix comments.

source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

So that it's only constructed once.
Should get us closest to the behaviour of a const global var.

source/ps/VisualReplay.h
48 ↗(On Diff #7882)

yeah, why not.

elexis added inline comments.May 11 2019, 11:21 PM
source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

Why should it only be constructed once?
Why should it get closest to the behavior of a constant global?

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1368/display/redirect

Imarok updated this revision to Diff 7975.May 11 2019, 11:29 PM

direct return

source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

I thought caching would be more efficient.
But don't really care.

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/differential/1369/display/redirect

elexis added inline comments.May 12 2019, 12:03 AM
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:
For non-slackware, first the global directory const is initialized with the default constructed g_args, then g_args is initialized with the correct value but the folder constant doesn't change.

(Hypothetically UserData() may also differ.)

Imarok added inline comments.May 12 2019, 12:25 AM
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.

elexis added inline comments.May 12 2019, 12:40 AM
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.
(While not currently the case, it's also not so far fetched that the main.cpp: g_args = args; call could be part of the init loop of RunGameOrAtlas.)

Imarok added inline comments.May 12 2019, 12:56 AM
source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

True.
So let's settle on caching in GetDirectoryPath() and not in the other functions?
Should be reasonable to me because we become immune to changed g_args and the other two functions don't need caching because they only depend on the now constant response of GetDirectoryPath().

elexis added inline comments.May 12 2019, 1:29 AM
source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

Why make the function return the wrong value in a more performant way?

Imarok added inline comments.May 12 2019, 1:31 AM
source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

Huh?
What do you mean with wrong value?
The old Paths(g_args).UserData() after g_args might have changed?

elexis added inline comments.May 12 2019, 1:44 AM
source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

Imarok added inline comments.May 12 2019, 1:49 AM
source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

Perfect. So the current patch should be fine for you?
{
(I thought the whole time that you think reacting to the changing g_args could lead to issues xD)

elexis added inline comments.May 12 2019, 12:05 PM
source/ps/VisualReplay.cpp
53 ↗(On Diff #7882)

The currently committed code is fine for me too because I'm not using slackware.

@weberkai I guess this newer version of the patch still fixes your issues?

I will test again and post the results ASAP.

Yes! I can build and run!
Very good!

This revision was automatically updated to reflect the committed changes.