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

Fix 'is_process_running' returns an incorrect 'true' when process 'pid' exists, but whose process does not belong to the current application. #75

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
78 changes: 66 additions & 12 deletions base/process.cpp
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// LAF Base Library
// Copyright (c) 2021 Igara Studio S.A.
// Copyright (c) 2021-2024 Igara Studio S.A.
// Copyright (c) 2015-2016 David Capello
//
// This file is released under the terms of the MIT license.
Expand All @@ -13,12 +13,20 @@

#if LAF_WINDOWS
#include <windows.h>
#include <tlhelp32.h>
#else
#include <signal.h>
#include <sys/types.h>
#include <unistd.h>
#endif

#if LAF_MACOS
#include <libproc.h>
#elif LAF_LINUX
#include "base/fs.h"
#include <cstring>
#endif

namespace base {

#if LAF_WINDOWS
Expand All @@ -28,34 +36,80 @@ pid get_current_process_id()
return (pid)GetCurrentProcessId();
}

bool is_process_running(pid pid)
std::string get_process_name(pid pid)
{
bool running = false;

HANDLE handle = OpenProcess(PROCESS_ALL_ACCESS, TRUE, pid);
HANDLE handle = CreateToolhelp32Snapshot(TH32CS_SNAPALL, 0);
Copy link
Member

Choose a reason for hiding this comment

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

From CreateToolhelp32Snapshot docs it looks like it's recommended to use QueryFullProcessImageNameW to get the process name. Probably we don't need this full snapshot thing (not sure what is the work this function does).

if (handle) {
DWORD exitCode = 0;
if (GetExitCodeProcess(handle, &exitCode)) {
running = (exitCode == STILL_ACTIVE);
PROCESSENTRY32 pe;
pe.dwSize = sizeof(PROCESSENTRY32);
if (Process32First(handle, &pe)) {
do {
char buf[64];
wcstombs(buf, pe.szExeFile, 64);
std::string str(buf);
for (char& c : str) {
c = tolower(c);
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't tolower the name, we should return the process name as it's reported and then post-process the name from the Aseprite side (remember that laf should be as independent from Aseprite implementation/details as possible).

}
if (pe.th32ProcessID == pid) {
return str;
}
} while (Process32Next(handle, &pe));
}
CloseHandle(handle);
}
return "";
Copy link
Member

Choose a reason for hiding this comment

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

Always remember to return an empty string as std::string instead of "" when the return value is std::string:

Suggested change
return "";
return std::string();

}

#elif LAF_MACOS

return running;
pid get_current_process_id()
{
return (pid)getpid();
}

#else // !LAF_WINDOWS
std::string get_process_name(pid pid)
{
struct proc_bsdinfo process;
proc_pidinfo(pid, PROC_PIDTBSDINFO, 0,
&process, PROC_PIDTBSDINFO_SIZE);
return process.pbi_name;
}

#elif LAF_LINUX

pid get_current_process_id()
{
return (pid)getpid();
}

bool is_process_running(pid pid)
std::string get_process_name(pid pid)
{
return (kill(pid, 0) == 0);
char path[128];
memset(path, 0, 128);
sprintf(path, "/proc/%d/exe", pid);
char* exepath = realpath(path, nullptr);
if (!exepath)
return "";

auto exename = base::get_file_name(exepath);
free(exepath);

return exename;
}

#endif

bool is_process_running(pid pid, std::string currentProcessName)
{
std::string pidProcessName = get_process_name(pid);
if (pidProcessName == "")
return false;
return pidProcessName == currentProcessName;
}

bool is_process_running(pid pid)
{
return false;
};
Comment on lines +102 to +113
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we have lost functionality from the laf side here: 1) the is_process_running does nothing (it should behave exactly as it was working), and 2) the new is_process_running(pid,name) looks like something could be done from the Aseprite side now that we have get_process_name.


} // namespace base
9 changes: 9 additions & 0 deletions base/process.h
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// LAF Base Library
// Copyright (c) 2023-2024 Igara Studio S.A.
// Copyright (c) 2015-2016 David Capello
//
// This file is released under the terms of the MIT license.
Expand All @@ -10,12 +11,20 @@

#include "base/ints.h"

#include <string>

namespace base {

typedef uint32_t pid;

pid get_current_process_id();

std::string get_process_name(pid pid);
Copy link
Member

@martincapello martincapello Jan 22, 2024

Choose a reason for hiding this comment

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

I think you should include <string> to avoid the errors reported by Github actions.


bool is_process_running(pid pid, std::string currentProcessName);

// Declaration to avoid errors during testing of Github actions
// TO DO: remove function after the implementation of PR aseprite/aseprite#4266
bool is_process_running(pid pid);

} // namespace base
Expand Down