Skip to content

Commit

Permalink
fix static variable client access vialotion in DamonConfigLoader (#965)
Browse files Browse the repository at this point in the history
Summary:
As discussed in pytorch issue [#129626](pytorch/pytorch#129626) , `updateThread` of `Config` will keep on accessing `client` after main thread quits.  But when main thread quits,  `client` will be destructed.  Functions in `updateThread` may use a dangling pointer of `client`, which will cause invalid memory access, and finally, `Segment fault` occurs, a core file will be generated, which  doesn't behave as expected.

Pull Request resolved: #965

Reviewed By: xuzhao9

Differential Revision: D60291572

Pulled By: aaronenyeshi

fbshipit-source-id: d97291aa825deeb672eb8e2d8385633c2838396d
  • Loading branch information
staugust authored and facebook-github-bot committed Jul 29, 2024
1 parent c2bc752 commit 188c5f5
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 4 deletions.
9 changes: 5 additions & 4 deletions libkineto/src/DaemonConfigLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,15 @@
#include "Logger.h"
#include "ConfigLoader.h"
#include "DaemonConfigLoader.h"
#include "IpcFabricConfigClient.h"

namespace KINETO_NAMESPACE {

// TODO : implications of this singleton being thread safe on forks?
IpcFabricConfigClient* getConfigClient() {
static auto client = std::make_unique<IpcFabricConfigClient>();
return client.get();
IpcFabricConfigClient* DaemonConfigLoader::getConfigClient() {
if (!configClient){
configClient = std::make_unique<IpcFabricConfigClient>();
}
return configClient.get();
}

std::string DaemonConfigLoader::readBaseConfig() {
Expand Down
7 changes: 7 additions & 0 deletions libkineto/src/DaemonConfigLoader.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@
#if !USE_GOOGLE_LOG
#include <memory>
#endif // !USE_GOOGLE_LOG
#ifdef __linux__
#include "IpcFabricConfigClient.h"
#endif // __linux__

namespace KINETO_NAMESPACE {

Expand Down Expand Up @@ -53,7 +56,11 @@ class DaemonConfigLoader : public IDaemonConfigLoader {

void setCommunicationFabric(bool enabled) override;

IpcFabricConfigClient* getConfigClient();

static void registerFactory();
private:
std::unique_ptr<IpcFabricConfigClient> configClient;
};
#endif // __linux__

Expand Down

0 comments on commit 188c5f5

Please sign in to comment.