-
Notifications
You must be signed in to change notification settings - Fork 33
Fix: undefined behavior in C++ implementation (Issue #236) #257
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: undefined behavior in C++ implementation (Issue #236) #257
Conversation
…inite loop issues (Issue ControlCore-Project#236)
|
@GaneshPatil7517 Please pay extra-attention when you create a PR. We need to test and create PRs against the dev branch. That way, your effort is not wasted (and also avoids the risk of the PR being mistakenly merged into the main branch). |
|
hey @pradeeban i immedatly switched to dev branch...... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR addresses correctness and portability issues in the core concore runtimes (notably the C++ shared-memory implementation), while also introducing a new Python CLI + packaging/CI/test scaffolding and migrating many scripts from print() to logging.
Changes:
- Fixes multiple undefined-behavior and infinite-loop risks in
concore.hpp(Linux shared-memory guarded, destructor safety, retry limits, empty-file guards). - Adds a new
concoreCLI (init/run/validate/inspect/status/stop) with packaging metadata and GitHub Actions CI/tests. - Updates Python/Docker runtimes and several
tools/scripts (logging, parameter parsing, ZMQ support, test coverage).
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tools/shannon.py | Replace print() with logging calls. |
| tools/pwrap.py | Replace print() with logging calls (includes request-loop diagnostics). |
| tools/plotymlag.py | Replace print() with logging calls. |
| tools/plotym.py | Replace print() with logging calls. |
| tools/plotu.py | Replace print() with logging calls. |
| tools/pidsig.py | Replace print() with logging calls for errors/info. |
| tools/pidmayuresh.py | Replace print() with logging calls. |
| tools/pid2.py | Replace print() with logging calls. |
| tools/learn.py | Replace print() with logging calls. |
| tools/cwrap.py | Replace print() with logging calls + minor URL logging formatting. |
| tools/cardiac_pm.py | Replace print() with logging calls + startup log. |
| tools/bangbang.py | Replace print() with logging calls + startup log. |
| tests/test_graph.py | Adds CLI validation tests for GraphML workflows. |
| tests/test_concoredocker.py | Adds tests for concoredocker.py I/O helpers and semantics. |
| tests/test_concore.py | Adds tests for concore.py helpers and param parsing. |
| tests/test_cli.py | Adds CLI smoke tests for init/run/validate/status. |
| tests/conftest.py | Adds pytest temp-dir fixture + path setup for tests. |
| tests/init.py | Marks tests as a package (empty). |
| startserver.bat | Adds Windows helper to start fri/server. |
| startserver | Adds POSIX helper to start fri/server. |
| setup.py | Adds setuptools packaging for CLI distribution. |
| requirements.txt | Adds CLI/runtime deps (e.g., pyzmq, click, rich, psutil). |
| requirements-dev.txt | Adds pytest tooling for development. |
| requirements-ci.txt | Adds minimal dependency set for CI. |
| pytest.ini | Adds pytest discovery/config defaults. |
| pyproject.toml | Adds PEP 517/518 + project metadata and console script entry point. |
| mkconcore.py | Adds input validation, safer file handling, and shell escaping in generated scripts. |
| contribute.py | Removes hardcoded token; uses env var for GitHub auth. |
| concoredocker.py | Adds logging, ZMQ support, param parsing, numpy conversion, and retry limits. |
| concoredocker.java | Expands Java runtime: parsing, retry limits, simtime double, Python-literal serialization. |
| concoredocker.hpp | Adds parsing helpers and simtime handling for Docker C++ runtime. |
| concore_read.m | Adds retry limit to prevent infinite loop on empty reads. |
| concore_cli/commands/validate.py | Implements workflow validation (XML/GraphML/node/edge checks). |
| concore_cli/commands/stop.py | Implements process stopping via psutil/taskkill/terminate. |
| concore_cli/commands/status.py | Implements process listing/status via psutil. |
| concore_cli/commands/run.py | Implements workflow generation and optional build execution. |
| concore_cli/commands/inspect.py | Adds workflow inspection (rich output + JSON output). |
| concore_cli/commands/init.py | Adds project scaffolding generation. |
| concore_cli/commands/init.py | Exposes command entry points. |
| concore_cli/cli.py | Adds Click-based CLI and command wiring. |
| concore_cli/init.py | Exposes CLI package entry. |
| concore_cli/README.md | Documents CLI usage and workflow format. |
| concore.py | Adds logging/ZMQ cleanup handling, param parsing, numpy conversion, and I/O changes. |
| concore.hpp | Fixes Linux shared-memory safety, retry limits, and empty-file guards. |
| TestLiteralEval.java | Adds standalone tests for the Java Python-literal parser. |
| README.md | Adds top-level CLI documentation section. |
| Dockerfile.java | Updates base image and adjusts jar copy logic. |
| .gitignore | Expands ignore rules for Python/IDE/testing artifacts. |
| .github/workflows/tests.yml | Adds Python test matrix workflow. |
| .github/workflows/ci.yml | Adds CI workflow (ruff + pytest + optional docker build). |
| .github/workflows/PR-review.yaml | Adds Gemini-based AI PR review workflow trigger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@GaneshPatil7517 yes, I noticed. It was one of those rarest moments when I am online and reviewing the PRs when someone is actively creating them! Good work. |
Thank you @pradeeban! It's my pleasure to contribute to this project. I'm committed to staying consistent and would love to continue collaborating with you on future improvements. From now on, I'll try to be more active and available for reviews. |
|
@GaneshPatil7517 my timezone is AKST. I work 6:15 am - 6:15 pm during Tues - Fri. I do not look at GitHub on Mondays usually and I do not work on weekends usually. |
|
Most of our mentors are based in Alaska. Some in EST (Atlanta or Lehigh) and many others in IST (India). A few other locations too (UK for one). |
|
@pradeeban yahh i see from today on wards ill come online at your time..... |
Summary
Fixes critical undefined behavior and cross-platform compatibility issues in
concore.hppas reported in Issue #236.Changes
Bug A — Destructor UB (CRITICAL)
shmId_createandshmId_getto-1sharedData_createandsharedData_gettonullptrshmdt()/shmctl()on uninitialized pointersBug B — POSIX-only headers
<sys/ipc.h>,<sys/shm.h>,<unistd.h>with#ifdef __linux__Bug C — mapParser() crash on empty port file
portstris empty to prevent index -1 accessBug D — Empty map iterator dereference
!iport.empty()and!oport.empty()guards before accessingbegin()Bug E — Infinite loop in read_FM(), read_SM(), getSharedMemory()
MAX_RETRY = 100limit to all retry loopsTesting
Closes #236