Skip to content

fix(bindings/C): fix the memory found in valgrind.#2673

Merged
Xuanwo merged 7 commits intoapache:mainfrom
xyjixyjixyji:fix_c_leak
Jul 20, 2023
Merged

fix(bindings/C): fix the memory found in valgrind.#2673
Xuanwo merged 7 commits intoapache:mainfrom
xyjixyjixyji:fix_c_leak

Conversation

@xyjixyjixyji
Copy link
Copy Markdown
Contributor

This PR fix the memory leak found in the tests in #2653.

I believe the memory leak is not completely fixed and it could be bettered through more tests on the C binding functionalities.

@github-actions github-actions Bot added the releases-note/fix The PR fixes a bug or has a title that begins with "fix" label Jul 19, 2023
@xyjixyjixyji
Copy link
Copy Markdown
Contributor Author

There is still an invalid read problem in the list test.

@xyjixyjixyji
Copy link
Copy Markdown
Contributor Author

xyjixyjixyji commented Jul 19, 2023

I think that the problem originates from the opendal_list_entry_path()

(*self.inner).path().as_ptr() as *const c_char

The path() returns a &str, which is a reference to the String typed path. And we are taking the pointer of that String directly. I am doing this because I don't want to wrap a opendal_string type, that seems overly verbose...... When I implemented that, I didn't think that was an issue since as long as the user calls opendal_list_entry_path(), the Entry must be alive and the String must be on heap.

I think the problem is that Rust String is not nul-terminated in memory..... Since valgrind reports this, I think we'd better fix this.

Now its fixed.

Copy link
Copy Markdown
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Xuanwo Xuanwo merged commit 442475e into apache:main Jul 20, 2023
@oowl oowl mentioned this pull request Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/fix The PR fixes a bug or has a title that begins with "fix"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants