Conversation
… and initialization
* Create artix4.txt * Rename artix4.txt to artix2.txt * Rename artix_small.txt to artix1_small.txt * Rename artix2_small.txt to artix_small.txt * Rename artix1_small.txt to artix2_small.txt
…nd simplify name list
* fix: colored inside shackle * Update ASCII art in secureblue.txt * Fix formatting in secureblue.txt --------- Co-authored-by: Carter Li <CarterLi@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 79 out of 79 changed files in this pull request and generated 6 comments.
You can also share your feedback on Copilot code review. Take the survey.
| LOGICAL_PROCESSOR_RELATIONSHIP lpr = RelationAll; | ||
| ULONG length = 0; | ||
| NtQuerySystemInformationEx(SystemLogicalProcessorAndGroupInformation, &lpr, sizeof(lpr), NULL, 0, &length); | ||
| if (length == 0) | ||
| return "GetLogicalProcessorInformationEx(RelationAll, NULL, &length) failed"; | ||
|
|
||
| SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX* FF_AUTO_FREE | ||
| pProcessorInfo = (SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX*)malloc(length); | ||
|
|
||
| if (!pProcessorInfo || !GetLogicalProcessorInformationEx(RelationAll, pProcessorInfo, &length)) | ||
| if (!NT_SUCCESS(NtQuerySystemInformationEx(SystemLogicalProcessorAndGroupInformation, &lpr, sizeof(lpr), pProcessorInfo, length, &length))) | ||
| return "GetLogicalProcessorInformationEx(RelationAll, pProcessorInfo, &length) failed"; |
There was a problem hiding this comment.
The failure messages in detectNCores() still mention GetLogicalProcessorInformationEx(...), but the code path now calls NtQuerySystemInformationEx(...). Updating the strings will keep diagnostics accurate.
| FFWmiVariant::FFWmiVariant(std::initializer_list<PCWSTR> strings): FFWmiVariant() | ||
| { | ||
| SAFEARRAYBOUND bound = { | ||
| .cElements = (ULONG) strings.size(), | ||
| .lLbound = 0, | ||
| }; | ||
| SAFEARRAY* psa = SafeArrayCreate(VT_BSTR, 1, &bound); | ||
|
|
||
| LONG i = 0; | ||
| for (PCWSTR str : strings) { | ||
| SafeArrayPutElement(psa, &i, bstr_t(str)); | ||
| ++i; | ||
| } | ||
|
|
||
| this->vt = VT_ARRAY | VT_BSTR; | ||
| this->parray = psa; | ||
| } |
There was a problem hiding this comment.
FFWmiVariant’s initializer_list constructor doesn’t check whether SafeArrayCreate() returned NULL, and it ignores SafeArrayPutElement() failures. In low-memory / COM failure scenarios this can lead to null dereferences or partially-initialized VARIANTs. Consider validating allocations/HRs and falling back to VT_EMPTY (or throwing/returning an error indicator) on failure.
| path->targetInfo.outputTechnology == DISPLAYCONFIG_OUTPUT_TECHNOLOGY_UDI_EMBEDDED | ||
| ? FF_DISPLAY_TYPE_BUILTIN : FF_DISPLAY_TYPE_EXTERNAL, | ||
| !!(monitorInfo->info.dwFlags & MONITORINFOF_PRIMARY), | ||
| (uint64_t)(uintptr_t) monitorInfo->handle, | ||
| sourceMode->position.x == 0 && sourceMode->position.y == 0, | ||
| path->sourceInfo.id, |
There was a problem hiding this comment.
Primary display detection is currently based on sourceMode->position.x == 0 && sourceMode->position.y == 0, which is not equivalent to “primary monitor” (layouts can place the primary at non-(0,0), and (0,0) can belong to a non-primary monitor in negative-coordinate layouts). Consider resolving the HMONITOR for this path and using GetMonitorInfoW(...).dwFlags & MONITORINFOF_PRIMARY, or otherwise using a DisplayConfig-backed primary indicator if available.
| SYSTEM_FIRMWARE_TABLE_INFORMATION sfti = { | ||
| .ProviderSignature = 'RSMB', | ||
| .Action = SystemFirmwareTableGet, | ||
| }; | ||
| ULONG bufSize = 0; | ||
| NtQuerySystemInformation(SystemFirmwareTableInformation, &sfti, sizeof(sfti), &bufSize); | ||
| if (bufSize <= sizeof(FFRawSmbiosData) + sizeof(sfti)) { | ||
| FF_DEBUG("Invalid firmware table size: %lu (must be > %zu)", bufSize, sizeof(FFRawSmbiosData) + sizeof(sfti)); | ||
| return NULL; | ||
| } | ||
| if (bufSize != sfti.TableBufferLength + (ULONG) sizeof(sfti)) { | ||
| FF_DEBUG("Firmware table size mismatch: NtQuerySystemInformation returned %lu but expected %lu", | ||
| bufSize, sfti.TableBufferLength + (ULONG) sizeof(sfti)); | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
The initial NtQuerySystemInformation(SystemFirmwareTableInformation, &sfti, sizeof(sfti), &bufSize) call is treated like a “size query”, but sfti.TableBufferLength is never set before it’s used in the subsequent size consistency check and copied into *buffer. If TableBufferLength is an input-only field (common for this info class), this will remain 0 and can make the size check fail and/or cause the second call to fail because buffer->TableBufferLength is still 0. Please set TableBufferLength explicitly (typically bufSize - sizeof(SYSTEM_FIRMWARE_TABLE_INFORMATION)) before the second call, and don’t rely on sfti.TableBufferLength being populated by the first call.
| @@ -19,40 +21,25 @@ bool ffGetFileVersion(const wchar_t* filePath, const wchar_t* stringName, FFstrb | |||
| UINT len; | |||
| if (VerQueryValueW(versionData, L"\\", (void **)&verInfo, &len) && len && verInfo->dwSignature == 0xFEEF04BD) | |||
| { | |||
| ffStrbufAppendF(version, "%u.%u.%u.%u", | |||
| (unsigned)((verInfo->dwProductVersionMS >> 16) & 0xffff), | |||
| (unsigned)((verInfo->dwProductVersionMS >> 0) & 0xffff), | |||
| (unsigned)((verInfo->dwProductVersionLS >> 16) & 0xffff), | |||
| (unsigned)((verInfo->dwProductVersionLS >> 0) & 0xffff)); | |||
| ffStrbufSetF(version, "%u.%u.%u.%u", | |||
| (unsigned)((verInfo->dwProductVersionMS >> 16) & 0xffff), | |||
| (unsigned)((verInfo->dwProductVersionMS >> 0) & 0xffff), | |||
| (unsigned)((verInfo->dwProductVersionLS >> 16) & 0xffff), | |||
| (unsigned)((verInfo->dwProductVersionLS >> 0) & 0xffff)); | |||
| return true; | |||
| } | |||
| } | |||
| else | |||
| { | |||
| struct | |||
| { | |||
| WORD language; | |||
| WORD codePage; | |||
| }* translations; | |||
|
|
|||
| UINT translationsLen; | |||
| wchar_t* value; | |||
| UINT valueLen; | |||
|
|
|||
| if (VerQueryValueW(versionData, L"\\VarFileInfo\\Translation", | |||
| (void **) &translations, &translationsLen) && | |||
| translationsLen >= sizeof(*translations)) | |||
| wchar_t subBlock[128] = L"\\StringFileInfo\\" FF_VERSION_LANG_EN_US L"\\"; | |||
| wcscat_s(subBlock, ARRAY_SIZE(subBlock), stringName); | |||
| if (VerQueryValueW(versionData, subBlock, (void **)&value, &valueLen) && valueLen > 0) | |||
| { | |||
| wchar_t subBlock[128]; | |||
| snwprintf(subBlock, ARRAY_SIZE(subBlock), L"\\StringFileInfo\\%04x%04x\\%ls", | |||
| translations[0].language, translations[0].codePage, stringName); | |||
|
|
|||
| wchar_t* value; | |||
| UINT valueLen; | |||
|
|
|||
| if (VerQueryValueW(versionData, subBlock, (void **)&value, &valueLen) && valueLen > 0) | |||
| { | |||
| ffStrbufSetWS(version, value); | |||
| return true; | |||
| } | |||
| ffStrbufSetWS(version, value); | |||
| return true; | |||
There was a problem hiding this comment.
ffGetFileVersion() now hard-codes the StringFileInfo language/codepage to 040904b0 (en-US). This can cause string queries (e.g. L"ProductVersion") to fail on binaries that only embed a different translation block. Consider falling back to \VarFileInfo\Translation (previous approach) or trying FF_VERSION_LANG_EN_US first and then iterating available translations if the query fails.
| NtQuerySystemInformationEx(SystemLogicalProcessorAndGroupInformation, &lpr, sizeof(lpr), NULL, 0, &length); | ||
| if (length == 0) | ||
| return "GetLogicalProcessorInformationEx(RelationCache, NULL, &length) failed"; | ||
|
|
||
| SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX* FF_AUTO_FREE | ||
| pProcessorInfo = (SYSTEM_LOGICAL_PROCESSOR_INFORMATION_EX*)malloc(length); | ||
|
|
||
| if (!pProcessorInfo || !GetLogicalProcessorInformationEx(RelationCache, pProcessorInfo, &length)) | ||
| if (!NT_SUCCESS(NtQuerySystemInformationEx(SystemLogicalProcessorAndGroupInformation, &lpr, sizeof(lpr), pProcessorInfo, length, &length))) | ||
| return "GetLogicalProcessorInformationEx(RelationCache, pProcessorInfo, &length) failed"; |
There was a problem hiding this comment.
The error strings still reference GetLogicalProcessorInformationEx(...), but the implementation now uses NtQuerySystemInformationEx(...). Updating these messages will make failures easier to diagnose (and avoid confusion when grepping logs).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 86 out of 86 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (10)
src/common/windows/version.c:1
- The string version lookup is now hard-coded to the
040904b0(en-US) language/codepage block. Many binaries don’t ship that translation, soffGetFileVersion(..., stringName, ...)can fail even when the value exists. Recommend restoring the\\VarFileInfo\\Translation-based lookup (try available translations first), and optionally fall back to040904b0only if no translations are present.
src/common/windows/version.c:1 - The string version lookup is now hard-coded to the
040904b0(en-US) language/codepage block. Many binaries don’t ship that translation, soffGetFileVersion(..., stringName, ...)can fail even when the value exists. Recommend restoring the\\VarFileInfo\\Translation-based lookup (try available translations first), and optionally fall back to040904b0only if no translations are present.
src/detection/displayserver/displayserver_windows.c:1 - Primary display detection via
position == (0,0)is incorrect: users can place a non-primary monitor at (0,0) or have negative virtual coordinates. This will mislabel the primary monitor. Prefer using a real primary flag (e.g.,GetMonitorInfoW(...).dwFlags & MONITORINFOF_PRIMARYon theHMONITORresolved for the display) rather than coordinates.
src/detection/cpucache/cpucache_windows.c:1 - The failure messages still reference
GetLogicalProcessorInformationEx(...), but the code now callsNtQuerySystemInformationEx(...). Update these strings so logs/errors identify the correct API and parameters (e.g., includeSystemLogicalProcessorAndGroupInformation).
src/detection/cpucache/cpucache_windows.c:1 - The failure messages still reference
GetLogicalProcessorInformationEx(...), but the code now callsNtQuerySystemInformationEx(...). Update these strings so logs/errors identify the correct API and parameters (e.g., includeSystemLogicalProcessorAndGroupInformation).
src/detection/cpu/cpu_windows.c:1 - Same issue as cpucache: error strings mention
GetLogicalProcessorInformationExwhile usingNtQuerySystemInformationEx. Please update the messages to match the new call path to avoid misleading diagnostics.
src/detection/cpu/cpu_windows.c:1 - Same issue as cpucache: error strings mention
GetLogicalProcessorInformationExwhile usingNtQuerySystemInformationEx. Please update the messages to match the new call path to avoid misleading diagnostics.
src/modules/title/title.c:1 - The Title module’s printed
{cwd}uses the tilde-rewritten buffer (cwdTilde), but the JSON result exports the rawinstance.state.platform.cwd. This makes{cwd}semantics inconsistent between text output and JSON (and contradicts the new module-info/schema description). Consider exporting the same tilde-rewritten value in JSON, or adjust the description/schema to explicitly state JSON uses the raw CWD.
src/common/windows/variant.cpp:1 SafeArrayCreatecan return NULL, andSafeArrayPutElementcan fail. Currently this will crash on NULL and leak/leave a partially constructed VARIANT on failures. Add checks forpsa == NULLandFAILED(SafeArrayPutElement(...)), and on failure destroy the SAFEARRAY (and keep the variant in a safe initialized state).
src/logo/builtin.c:1- Several logo entries remove previously accepted aliases (e.g.,
artixlinux,artix-linux,debian-linux, etc.). This can break existing user configs that reference those names. Consider keeping backward-compatible aliases in.names(even if you prefer a canonical ID) unless there’s a strong reason to drop them.
You can also share your feedback on Copilot code review. Take the survey.
| src/detection/netio/netio_windows.c | ||
| src/detection/opengl/opengl_windows.c | ||
| src/detection/os/os_windows.cpp | ||
| src/detection/os/os_windows.c |
There was a problem hiding this comment.
CMakeLists now references src/detection/os/os_windows.c, but the diff shows changes to src/detection/os/os_windows.cpp (and no added os_windows.c). As-is, this is likely a build break on Windows. Fix by either renaming the source file to os_windows.c (and updating the repository accordingly) or keeping the CMake entry as os_windows.cpp.
| src/detection/os/os_windows.c | |
| src/detection/os/os_windows.cpp |
| #if _WIN64 | ||
| static_assert( | ||
| offsetof(RTL_USER_PROCESS_PARAMETERS, Reserved2[5]) == 0x38, | ||
| "CurrentDirectory should be at offset 0x38 in RTL_USER_PROCESS_PARAMETERS. Structure layout mismatch detected."); | ||
| #endif | ||
| PCURDIR cwd = (PCURDIR) &NtCurrentTeb()->ProcessEnvironmentBlock->ProcessParameters->Reserved2[5]; | ||
| ffStrbufSetNWS(&platform->cwd, cwd->DosPath.Length / sizeof(WCHAR), cwd->DosPath.Buffer); | ||
| ffStrbufReplaceAllC(&platform->cwd, '\\', '/'); | ||
| ffStrbufEnsureEndsWithC(&platform->cwd, '/'); | ||
| } | ||
|
|
There was a problem hiding this comment.
This reads the current directory via undocumented/internals (Reserved2[5] layout), which is brittle across OS versions/architectures (and isn’t validated for 32-bit). Since this runs only at init time, prefer the documented GetCurrentDirectoryW approach (then normalize slashes) to avoid relying on internal structure offsets.
| #if _WIN64 | |
| static_assert( | |
| offsetof(RTL_USER_PROCESS_PARAMETERS, Reserved2[5]) == 0x38, | |
| "CurrentDirectory should be at offset 0x38 in RTL_USER_PROCESS_PARAMETERS. Structure layout mismatch detected."); | |
| #endif | |
| PCURDIR cwd = (PCURDIR) &NtCurrentTeb()->ProcessEnvironmentBlock->ProcessParameters->Reserved2[5]; | |
| ffStrbufSetNWS(&platform->cwd, cwd->DosPath.Length / sizeof(WCHAR), cwd->DosPath.Buffer); | |
| ffStrbufReplaceAllC(&platform->cwd, '\\', '/'); | |
| ffStrbufEnsureEndsWithC(&platform->cwd, '/'); | |
| } | |
| wchar_t cwdW[MAX_PATH]; | |
| DWORD cwdLen = GetCurrentDirectoryW(MAX_PATH, cwdW); | |
| if (cwdLen == 0 || cwdLen >= MAX_PATH) | |
| return; | |
| ffStrbufSetNWS(&platform->cwd, cwdLen, cwdW); | |
| ffStrbufReplaceAllC(&platform->cwd, '\\', '/'); | |
| ffStrbufEnsureEndsWithC(&platform->cwd, '/'); | |
| } |
src/common/impl/kmod_windows.c
Outdated
| #include "common/mallocHelper.h" | ||
| #include "common/stringUtils.h" | ||
|
|
||
| bool ffKmodLoaded(FF_MAYBE_UNUSED const char* modName) |
There was a problem hiding this comment.
modName is used in this function, but it’s annotated FF_MAYBE_UNUSED. While harmless, it’s misleading and can mask real unused-parameter issues elsewhere. Consider removing the FF_MAYBE_UNUSED annotation here.
| bool ffKmodLoaded(FF_MAYBE_UNUSED const char* modName) | |
| bool ffKmodLoaded(const char* modName) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 87 out of 87 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/common/impl/binary_windows.c:59
ffBinaryExtractStrings()maps the PE and then usessection->PointerToRawData/SizeOfRawDataplusstrlen(p)without any bounds checking against the mapped file size. A malformed or truncated PE could cause out-of-bounds reads (includingstrlenscanning past the section), leading to crashes and potential security issues when scanning user-provided files. Consider validating that the section range is within the mapped view and replacingstrlen(p)with a bounded scan (limited toSizeOfRawData - off).
PIMAGE_SECTION_HEADER section = IMAGE_FIRST_SECTION(ntHeaders);
for (WORD i = 0; i < ntHeaders->FileHeader.NumberOfSections; ++i, ++section)
{
// Look for initialized data sections with the name ".rdata" which typically contains string literals
if ((section->Characteristics & IMAGE_SCN_CNT_INITIALIZED_DATA) && ffStrEquals((const char*) section->Name, ".rdata"))
{
uint8_t *data = (uint8_t *) base + section->PointerToRawData;
// Scan the section for string literals
for (size_t off = 0; off < section->SizeOfRawData; ++off)
{
const char* p = (const char*) data + off;
if (*p == '\0') continue;
uint32_t len = (uint32_t) strlen(p);
if (len < minLength) continue;
// Only process printable ASCII characters
if (*p >= ' ' && *p <= '~') // Ignore control characters
{
if (!cb(p, len, userdata)) break;
}
off += len;
You can also share your feedback on Copilot code review. Take the survey.
| yyjson_mut_obj_add_strbuf(doc, obj, "exePath", &instance.state.platform.exePath); | ||
| yyjson_mut_obj_add_strbuf(doc, obj, "userShell", &instance.state.platform.userShell); | ||
| yyjson_mut_obj_add_uint(doc, obj, "pid", instance.state.platform.pid); | ||
| yyjson_mut_obj_add_strbuf(doc, obj, "cwd", &instance.state.platform.cwd); |
There was a problem hiding this comment.
The {cwd} format arg is documented as “CWD with home dir replaced by ~” and ffPrintTitle() passes the tilde-normalized value, but the JSON result currently exports the raw instance.state.platform.cwd. This mismatch can surprise consumers that rely on JSON output matching the formatting semantics. Consider either exporting the tilde-normalized value here as well, or adding a separate field (e.g., cwdRaw vs cwd).
No description provided.