Skip to content

Fix: Memory leak in riscv_openocd_step_impl() #1303

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 7 commits into
base: riscv
Choose a base branch
from
Open

Fix: Memory leak in riscv_openocd_step_impl() #1303

wants to merge 7 commits into from

Conversation

@Biancaa-R
Copy link

@Biancaa-R Biancaa-R commented Oct 27, 2025 •
edited
Loading

cleared wps_to_enable in _exit after there is no further reference to it .
The memory allocated at :
image

_exit:
	if (enable_watchpoints(target, wps_to_enable) != ERROR_OK) {
		success = false;
		LOG_TARGET_ERROR(target, "Failed to re-enable watchpoints "
				"after single-step.");
	}
	free(wps_to_enable);
	//no longer needed to be kept

Fixes the issue: #1284

  • Working on the Valgrind part ,will add soon .

Biancaa Ramesh and others added 4 commits October 27, 2025 20:27
added the necessary tests for progbufsize
51db0ca 
Added additional debug logs to verify correct retrieval and printing
of progbufsize in various code paths within riscv-013.c. This helps
in debugging hardware configurations that expose custom program buffer
sizes.

Signed-off-by: Biancaa Ramesh <[email protected]>
riscv: Add have_progbuf_size check in vtype,vl writes and crs,fpr rea…
38450ad 
…d-write

Added a check for have_progbuf_size before attempting to execute. This prevents program buffer access on targets
that do not support it, improving robustness and avoiding potential
execution failures when the program buffer is not available or too small.

Signed-off-by: Biancaa Ramesh <[email protected]>
riscv: Fix memory leak in riscv_openocd_step_impl
8557f4f 
Fixes a memory leak caused by wps_to_enable being allocated with calloc()
and never freed. The pointer is now properly freed at the end of the
function and on early error returns. This leak was originally introduced
in commit 5a8697b (trigger management refactor).

Signed-off-by: Biancaa Ramesh [email protected]
@Biancaa-R
original file _ will rebase it
a08870a 
Signed-off-by: Biancaa Ramesh <[email protected]>
@en-sc
Copy link
Collaborator

en-sc commented Oct 27, 2025

@Biancaa-R, thank you for the patch.

Unfortunately, there is already a patch with a fix. I should have mentioned it in the issue, sorry.
See https://review.openocd.org/c/openocd/+/9163

@en-sc
Copy link
Collaborator

en-sc commented Oct 27, 2025

Working on the Valgrind part ,will add soon.

Are you referring to introducing test runs under Valgrind to the CI?

@Biancaa-R
Copy link
Author

Biancaa-R commented Oct 28, 2025

Working on the Valgrind part ,will add soon.

Are you referring to introducing test runs under Valgrind to the CI?

Yes I am ,to ensure it is caught .

@Biancaa-R
Copy link
Author

Biancaa-R commented Oct 28, 2025

@Biancaa-R, thank you for the patch.

Unfortunately, there is already a patch with a fix. I should have mentioned it in the issue, sorry. See https://review.openocd.org/c/openocd/+/9163

Oh no worries :)

@Biancaa-R
Update spike-openocd-tests.yml
c362895 
This adds a lightweight run that:

Starts OpenOCD once under Valgrind.

Runs initialization and shutdown.

Fails if Valgrind reports any memory leaks (via --error-exitcode=1).

Signed-off-by: Biancaa Ramesh <[email protected]>
@Biancaa-R
Copy link
Author

Biancaa-R commented Nov 7, 2025 •
edited
Loading

Ok let me check and get back.

@Biancaa-R
Update spike-openocd-tests.yml
97d76f6 
Signed-off-by: Biancaa Ramesh <[email protected]>
@Biancaa-R
Update spike-openocd-tests.yml
fd51cc5 
Tried to run valgrind with no dependent cfg files.

Signed-off-by: Biancaa Ramesh <[email protected]>
@Biancaa-R
Copy link
Author

Biancaa-R commented Nov 9, 2025

If this approach also doesnt work ,I would probably :
Run Valgrind only through the internal TCL interpreter

If our goal is just to check core memory management (no targets at all):

valgrind --leak-check=full --error-exitcode=1
./src/openocd -c "shutdown"

Or use a dummy config file additionally.

@en-sc
Copy link
Collaborator

en-sc commented Nov 10, 2025

@Biancaa-R, I'd suggest not to spend too much time on this one right now. We are currently merging RISC-V OpenOCD with the mainline and after this work is done, I'd like to stop the development in this repo, posting patches to https://review.openocd.org instead.
So, the workflow will need to be adjusted (mainline OpenOCD uses Gerrit and Jenkins).

As for what can be run under valgrind -- there is the make check and the tests from testing/tcl_commands that are already part of the mainline but are not yet merged into the fork. See 1b2a2b8 and #1290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@Biancaa-R @en-sc