Update to version 0.9.2#1
Conversation
|
@copilot please review this pull request. Group your findings and order them according to their criticality. |
edmc25
force-pushed
the
update_to_version_0.9.2
branch
from
b642002 to
224dbf0
Compare
Here is my review of this PR, grouped by criticality: 🔴 Critical — Build-Breaking: Incomplete Caller Updates for Signature ChangesSeveral functions were given additional
|
There was a problem hiding this comment.
Pull request overview
This PR updates the project to version 0.9.2 and introduces a set of “safer” utility wrappers for string formatting/copying and memory operations, then applies those wrappers across scheduler/global/external-process code. It also extends parts of scheduler stop-control and A2L conversion functionality, plus updates build configuration and sample assets.
Changes:
- Add new utilities (
PrintFormatToString.*,MemZeroAndCopy.h) and adopt them broadly (replacingsprintf/strcpy/memset/memcpypatterns). - Update multiple APIs to be buffer-length aware (e.g.,
CheckOpenIPCFile, several scheduler/process-name helpers). - Extend scheduler stop requests with an optional equation condition; expand A2L conversion capabilities and related data structures; update build + samples for 0.9.2.
Reviewed changes
Copilot reviewed 111 out of 111 changed files in this pull request and generated 21 comments.
Show a summary per file
| File | Description |
|---|---|
| Src/Utilities/StringMaxChar.h | Adds array-safe string copy/append helper macros; moves mem helpers into MemZeroAndCopy.h. |
| Src/Utilities/PrintFormatToString.h | Declares safe printf-to-buffer helpers. |
| Src/Utilities/PrintFormatToString.c | Implements formatting helpers using vsnprintf/vsnprintf_s. |
| Src/Utilities/MemZeroAndCopy.h | Introduces MEMSET/MEMCPY/ZEROMEM and struct zero-init macros. |
| Src/Utilities/CMakeLists.txt | Adds new utility source to targets; reorganizes common file lists. |
| Src/Scheduler/VirtualNetwork.c | Replaces raw memset/memcpy with project macros. |
| Src/Scheduler/UnixDomainSocketMessages.c | Uses safer path creation/copy; adds thread accept-handshake wait. |
| Src/Scheduler/SocketMessages.c | Uses safer zero-init/copy; adds thread accept-handshake wait. |
| Src/Scheduler/SharedDataTypes.h | Adds new blackboard conversion enum values. |
| Src/Scheduler/Scheduler.h | Adds fields and expands multiple APIs with Maxc parameters; adds equation param to cycle control. |
| Src/Scheduler/SchedEnableDisable.h | Extends timed stop request to carry an execution stack / equation. |
| Src/Scheduler/SchedEnableDisable.c | Implements equation-based timed stop requests; swaps to MEMSET. |
| Src/Scheduler/SchedBarrier.h | Makes GetNextBarrierLoggingEntry buffer-size aware. |
| Src/Scheduler/SchedBarrier.c | Replaces unsafe string ops/sprintf; updates barrier logging API usage. |
| Src/Scheduler/ScBbCopyLists.c | Replaces sprintf/memset; initializes locals; uses safe formatting. |
| Src/Scheduler/ProcessEquations.c | Uses StringCopyMaxCharTruncate for error aggregation. |
| Src/Scheduler/PipeMessages.c | Uses safe formatting/copy; adds thread accept-handshake wait. |
| Src/Scheduler/KillAllExternProcesses.c | Uses safe formatting for event name. |
| Src/Scheduler/ExternLoginTimeoutControl.c | Uses StringMalloc instead of manual malloc+strcpy. |
| Src/Scheduler/ExtProcessRefFilter.c | Uses safe formatting. |
| Src/Scheduler/BaseMessages.c | Propagates new max-length APIs; safer formatting/copy. |
| Src/Global/my_udiv128.c | Changes divide-by-zero behavior handling. |
| Src/Global/WindowIniHelper.c | Uses safe formatting/copy; fixes return value logic. |
| Src/Global/Wildcards.c | Reworks wildcard matching; replaces unsafe string copies; uses safe formatting. |
| Src/Global/UtilsWindow.c | Switches to array-safe append macro. |
| Src/Global/UniqueNumber.c | Comment fix. |
| Src/Global/TimeProcess.c | Uses gmtime_r; initializes values on init; changes task period. |
| Src/Global/ThrowError.h | Renames ThrowErrorWiithCycle to ThrowErrorWithCycle. |
| Src/Global/ThrowError.c | Uses safe formatting; refactors header allocation; updates renamed function. |
| Src/Global/StartupInit.c | Adds equation parser init; switches to safe copying. |
| Src/Global/StartExeAndWait.c | Uses MEMSET and safe formatting. |
| Src/Global/RunTimeMeasurement.c | Replaces memset/sprintf with project wrappers. |
| Src/Global/ReplaceFuncWithProg.c | Uses safe string copy/append for command line construction. |
| Src/Global/Platform.h | Adds Sleep for linux wrapper; makes IPC/home-dir APIs max-length aware. |
| Src/Global/Platform.c | Implements linux Sleep; updates IPC path building to safe string ops. |
| Src/Global/ParseCommandLine.c | Uses safe append/copy and StringMalloc. |
| Src/Global/MyMemory.h | Removes write_memory_infos_to_file prototype. |
| Src/Global/MyMemory.c | Uses MEMSET; removes write_memory_infos_to_file implementation. |
| Src/Global/MainValues.h | Adds HideControlPanelLock. |
| Src/Global/MainValues.c | Uses safe formatting/memset; initializes new field. |
| Src/Global/InitProcess.c | Uses safe formatting/copy; adjusts version vars; other init tweaks. |
| Src/Global/IniFileDontExist.c | Uses safe formatting/copy and new home-dir signature. |
| Src/Global/ImExportVarProperties.c | Uses struct zero-init macro; extends conversion cases handled. |
| Src/Global/ImExportDskFile.c | Uses safe formatting/copy; removes ignored return checks. |
| Src/Global/Files.h | Removes legacy commented block. |
| Src/Global/Files.c | Uses StringMalloc and safe formatting/copy in file helpers. |
| Src/Global/Fifos.c | Uses safe copy; removes unused debug var. |
| Src/Global/EnvironmentVariables.c | Uses safe formatting/copy; removes old special-case mutation block. |
| Src/Global/Config.h | Bumps patch version to 2. |
| Src/Global/CheckIfAlreadyRunning.c | Uses safe formatting; updates IPC signature. |
| Src/Global/CMakeLists.txt | Reorganizes linux sources; adds Platform.c to additional target. |
| Src/ExternalProcess/XilEnvExtProcMain.c | Fixes command-line parsing / dll path extraction logic; safer copy. |
| Src/ExternalProcess/XilEnvExtProc.h | Bumps patch version; replaces sprintf with safe formatting in macros. |
| Src/ExternalProcess/OpenXilEnvExtp.def | Changes exported symbol (removes SetHwndMainWindow, adds GetSchedulingInformation). |
| Src/ExternalProcess/ExtpXcpCopyPages.cpp | Uses MEMSET; makes helper APIs buffer-size aware; safe formatting/copy. |
| Src/ExternalProcess/ExtpVirtualNetwork.c | Uses MEMSET. |
| Src/ExternalProcess/ExtpUnixDomainSocketMessages.c | Uses safe IPC + copy; uses MEMSET. |
| Src/ExternalProcess/ExtpSocketMessages.c | Uses safe copy/format; uses MEMSET. |
| Src/ExternalProcess/ExtpReferenceVariables.c | Uses safe formatting; uses StringMalloc; uses MEMSET. |
| Src/ExternalProcess/ExtpProcessAndTaskInfos.h | Removes stored main window handle field. |
| Src/ExternalProcess/ExtpPipeMessages.c | Uses safe formatting for pipe name. |
| Src/ExternalProcess/ExtpParseCmdLine.c | Uses StringCopyMaxCharTruncate. |
| Src/ExternalProcess/ExtpMain.c | Removes SetHwndMainWindow call and exit(0). |
| Src/ExternalProcess/ExtpKillExternProcessEvent.c | Uses safe formatting/copy; alloc length fix. |
| Src/ExternalProcess/ExtpExtError.c | Uses safe copy/append; improves logging setup. |
| Src/ExternalProcess/ExtpBlackboardCopyLists.c | Fixes string allocation size; uses safe copy. |
| Src/ExternalProcess/ExtpBlackboard.c | Adjusts cache allocation; uses safe copy; uses MEMSET. |
| Src/ExternalProcess/ExtpBaseMessages.c | Removes SetHwndMainWindow; uses safe copy/append; linux SIGPIPE handling. |
| Src/CMakeLists.txt | Reorders Utilities add_subdirectory. |
| Src/A2lParser/A2LUpdate.c | Uses safe formatting/copy and struct zero-init. |
| Src/A2lParser/A2LTokenizer.c | Uses struct zero-init; removes dead code block. |
| Src/A2lParser/A2LParser.c | Uses safe formatting/copy and new varargs formatting wrapper. |
| Src/A2lParser/A2LLinkThread.h | Adds LinkNo. |
| Src/A2lParser/A2LLinkThread.c | Adds <inttypes.h>. |
| Src/A2lParser/A2LLink.h | Adds new alignment flag + import/export measurement refs declarations. |
| Src/A2lParser/A2LData.h | Changes/adds reference counters/flags/vid fields. |
| Src/A2lParser/A2LConvertToXcp.c | Uses safe formatting; changes ini open and section naming. |
| Src/A2lParser/A2LConvert.c | Adds conversion-by-table/interpolation and rational conversion changes. |
| Src/A2lParser/A2LBuffer.c | Uses struct zero-init. |
| Src/A2lParser/A2LAccessData.h | Changes Dup signature; adds equality comparator declaration. |
| Src/A2lParser/A2LAccessData.c | Adds default alignment option; uses MEMCPY; implements Dup/equality compare. |
| Samples/ExternalProcesses/ExtProc_OpenScenario/CMakeLists.txt | Renames sample source file. |
| Samples/ExternalProcesses/ExtProc_FMUExtract/FmuExtract.cpp | Fixes directory attribute check; adds error handling on extract. |
| Samples/ExternalProcesses/ExtProc_FMU3Loader/Fmu3Struct.h | Adds logging fields; includes stdio. |
| Samples/ExternalProcesses/ExtProc_FMU3Loader/Fmu3Execute.cpp | Adds FMU logging and uses scheduler period for timestep. |
| Samples/ExternalProcesses/ExtProc_FMU3Loader/ExtProc_FMU3Loader.cpp | Adds env-driven FMU logging config. |
| Samples/ExternalProcesses/ExtProc_FMU2Loader/Fmu2Struct.h | Adds logging fields; includes stdio. |
| Samples/ExternalProcesses/ExtProc_FMU2Loader/Fmu2Execute.cpp | Adds FMU logging; uses scheduler period for timestep; fixes boolean input set bug. |
| Samples/ExternalProcesses/ExtProc_FMU2Loader/ExtProc_FMU2Loader.cpp | Adds env-driven parameter referencing + logging; removes debug block. |
| Samples/ExternalProcesses/ExtProc_BatteryModel/ExtProc_BatteryModel.c | Adds random/pulse signals for sample. |
| Samples/Configurations/ElectricCarSampleDebug.ini | Updates sample GUI/layout/vars; bumps file version to 0.9.2. |
| CMakeLists.txt | Adds optional QtSvg support and link adjustments; adds dl on linux. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define STRING_COPY_TO_ARRAY_OFSET(Dst, Offset, Src) static_assert(sizeof(Dst) > sizeof(void*), "this should be an array that is larger than a pointer");\ | ||
| StringCopyMaxCharTruncate((Dst) + (Offset), Src, sizeof(Dst) - (Offset)) |
There was a problem hiding this comment.
Macro name typo: STRING_COPY_TO_ARRAY_OFSET is likely intended to be STRING_COPY_TO_ARRAY_OFFSET. This will be easy to misuse and hurts API discoverability; consider renaming (and optionally keeping a deprecated alias for compatibility).
| #ifdef _WIN32 | ||
| Ret = vsnprintf_s (ret_DestBuffer, par_SizeOfDestBuffer, par_SizeOfDestBuffer, par_Format, vlist); | ||
| #else | ||
| Ret = vsnprintf (ret_DestBuffer, par_SizeOfDestBuffer, par_Format, vlist); | ||
| #endif |
There was a problem hiding this comment.
On Windows, vsnprintf_s expects the third argument (count) to be <= sizeOfBuffer-1 or _TRUNCATE. Passing par_SizeOfDestBuffer for both sizeOfBuffer and count can trigger the invalid-parameter handler at runtime. Use _TRUNCATE (or par_SizeOfDestBuffer-1) for the count argument to get safe truncation behavior consistent with the non-Windows path.
| RuntimeInfosSize += 64; | ||
| RuntimeInfos = (RUNTIME_INFOS*)my_realloc (RuntimeInfos, RuntimeInfosSize * sizeof (RUNTIME_INFOS)); | ||
| } | ||
| memset (&(RuntimeInfos[id]), 0, sizeof (RUNTIME_INFOS)); | ||
| MEMSET (&(RuntimeInfos[id]), 0, sizeof (RUNTIME_INFOS)); | ||
| } |
There was a problem hiding this comment.
MEMSET is used here, but this file does not include the header that defines MEMSET (MemZeroAndCopy.h). PrintFormatToString is also used later without including PrintFormatToString.h. Please add the required includes (or stop using the macros here) to avoid implicit-declaration/undefined identifier build errors.
| Ret = 1; // Should be stopped | ||
| par_Requests->SchedulerDisabledFlag = 1; | ||
| if (par_Requests->StopRequestElems[x].ExecStack != NULL) { | ||
| remove_exec_stack(par_Requests->StopRequestElems[x].ExecStack); | ||
| par_Requests->StopRequestElems[x].ExecStack = NULL; |
There was a problem hiding this comment.
Out-of-bounds access: after the for-loop, x == OpenStopRequestCount, but StopRequestElems[x] is accessed here. This can read/write past the active array. ExecStack cleanup should happen when removing each element (inside the removal loop), not by indexing after the loop.
| // wait until the login thread has called accept() | ||
| for(int x = 0; (x < 10) && !ThreadWillCallFirstAcceptState; x++) { | ||
| Sleep(10); | ||
| } | ||
| Sleep(10); |
There was a problem hiding this comment.
The post-thread-start wait loop relies on ThreadWillCallFirstAcceptState without proper synchronization (atomic/condvar), so it can spin/sleep unnecessarily or miss the update. Prefer signaling readiness via a condition variable (or at least an atomic with proper memory ordering).
| #endif | ||
|
|
||
| int ThrowErrorWiithCycle (int level, uint64_t Cycle, const char *format, ...); | ||
| int ThrowErrorWithCycle (int level, uint64_t Cycle, const char *format, ...); |
There was a problem hiding this comment.
This header renames ThrowErrorWiithCycle -> ThrowErrorWithCycle, but at least one call site still uses the old symbol (e.g. Src/RemoteMaster/Client/RemoteMasterControlProcess.c:163). Either update remaining call sites or provide a temporary compatibility macro/alias to avoid link/build failures.
| case 1: // COMPU_VTAB | ||
| case 2: // COMPU_VTAB_RANGE | ||
| default: | ||
| return -1; | ||
| break; |
There was a problem hiding this comment.
ConvertRawToPhysByTable: case 0 falls through into the default branch that returns -1 (missing break/return). This prevents TAB_INTP/TAB_NOINTP raw->phys conversion from ever reporting success.
| static int ThreadWillCallFirstAcceptState; | ||
|
|
There was a problem hiding this comment.
ThreadWillCallFirstAcceptState is used for cross-thread signaling without atomic/volatile or a condition variable. This is a C data race; the waiting thread may never observe updates reliably. Use an atomic flag or a mutex+condition-variable handshake.
| A2L_DATA* Src = (A2L_DATA*)par_Data; | ||
| A2L_DATA* Ret; | ||
| if (Src != NULL) { | ||
| Ret = (A2L_DATA*)A2L_DATA_MALLOC(Src->StructSize); |
There was a problem hiding this comment.
DupA2lData copies into Ret without checking whether A2L_DATA_MALLOC succeeded. If allocation fails, MEMCPY will dereference NULL. Add a NULL check before copying and return NULL on OOM.
| Ret = (A2L_DATA*)A2L_DATA_MALLOC(Src->StructSize); | |
| Ret = (A2L_DATA*)A2L_DATA_MALLOC(Src->StructSize); | |
| if (Ret == NULL) { | |
| return NULL; // out of memory | |
| } |
| // wait until the login thread has called accept() | ||
| for(int x = 0; (x < 10) && !ThreadWillCallFirstAcceptState; x++) { | ||
| usleep(10*1000); | ||
| } | ||
| usleep(10*1000); |
There was a problem hiding this comment.
The wait loop here uses ThreadWillCallFirstAcceptState as a readiness signal without synchronization (C data race). Use a condition variable or atomic flag to avoid undefined behavior and flaky startup timing.
edmc25
force-pushed
the
update_to_version_0.9.2
branch
from
224dbf0 to
8b361b2
Compare
edmc25
force-pushed
the
update_to_version_0.9.2
branch
from
8b361b2 to
dfebef4
Compare
|
@copilot You are an expert code reviewer. Your task is to thoroughly review the provided pull request and deliver a structured, actionable report. Your Review ScopeExamine the following dimensions:
Output FormatGroup and sort your findings into the following criticality tiers, in this order: 🔴 Critical (Must fix before merge)Issues that cause crashes, data loss, security vulnerabilities, or broken functionality. 🟠 Major (Should fix before merge)Significant bugs, logic errors, or design issues that will likely cause problems in production. 🟡 Minor (Consider fixing)Code quality issues, suboptimal patterns, or missing tests that don't block functionality but reduce maintainability. 🔵 Nit (Optional / Low priority)Style inconsistencies, minor naming suggestions, or cosmetic improvements. For each finding, use this structure:
At the top of your review, provide a short Summary (3–5 sentences) covering:
Do not praise the author or add filler commentary. Be direct, specific, and constructive. |
… caller updates, resource leaks Agent-Logs-Url: https://github.com/edmc25/openxilenv/sessions/1a80c6df-c203-4333-8a22-05a330582ed8 Co-authored-by: edmc25 <198079695+edmc25@users.noreply.github.com>
... Here is the structured review. All actionable issues have been fixed in commit Summary: This PR is a significant update introducing safer string/memory utility wrappers and applying them broadly, extending A2L conversion, and adding scheduler stop-by-equation support. The most critical concern is a large set of API signature changes where buffer-size parameters were added to many functions, but dozens of callers were left unupdated (build-breaking). Additionally, several logic bugs were introduced in the new A2L conversion code. All issues below have been fixed. Recommend: Request Changes → all addressed in 🔴 Critical (Must fix before merge)1. Build-breaking:
2. Build-breaking: Buffer-size parameters added to ~10 functions, callers not updated
3. Buffer overflow in
4. Missing
5. Same fall-through bug in
6. TAB_INTP/TAB_NOINTP falls through to
7. Out-of-bounds access in
8. ExecStack memory leak in
9. NULL dereference in
🟠 Major (Should fix before merge)10.
11. Error log for
12.
🟡 Minor (Consider fixing)13. Missing includes in
14.
15.
🔵 Nit (Optional / Low priority)16. CMakeLists.txt typo:
17.
18.
|
…ript, StimulusPlayer, TraceRecorder and XcpOverEthernet files; add ToParseFileStack.cpp, ConvertDbcToCanCmd.cpp, ExportA2lMeasurementListCmd.cpp and ImportA2lMeasurementListCmd.cpp
|
@copilot Please review the latest commit following the same instructions as in your last review. |
…CanCmd.cpp unsafe sprintf → PrintFormatToString Agent-Logs-Url: https://github.com/edmc25/openxilenv/sessions/46efbdf9-231b-4216-8baa-d58677895432 Co-authored-by: edmc25 <198079695+edmc25@users.noreply.github.com>
Review completed — see my detailed reply to the earlier review request comment (same thread). All actionable bugs found in commit
Remaining findings (Major items eclipse-openxilenv#3-5, Minor eclipse-openxilenv#6-8, Nits eclipse-openxilenv#9-12) are documented in the detailed review and may be addressed at your discretion. |
💬 Send tasks to Copilot coding agent from Slack and Teams to turn conversations into code. Copilot posts an update in your thread when it's finished.