Skip to content

fix: Vec.as_ptr() might return a dangling pointer#760

Merged
zshipko merged 1 commit intoextism:mainfrom
evacchi:fix-dangling-ptr
Sep 4, 2024
Merged

fix: Vec.as_ptr() might return a dangling pointer#760
zshipko merged 1 commit intoextism:mainfrom
evacchi:fix-dangling-ptr

Conversation

@evacchi
Copy link
Copy Markdown
Contributor

@evacchi evacchi commented Aug 30, 2024

Both as_ptr and as_mut_ptr are allowed to return a dangling raw pointer when the Vec size is 0.

The idea is that you should guard that read checking the size. This probably works well in most cases, but at the very least in the java-sdk, the JNA machinery tries to be helpful and it dereferences the pointer, causing a SIGSEGV.

The solution is to check if the resulting vector is empty and return null instead. A new, empty vector would be better, but I think that would not solve the problem, because the problem is caused by a new, empty vector in the first place.

Caveat: this might break consumers downstream.
On the other hand: consumers that do not check for the nInput, nOutput counts are just waiting to explode, like JNA.

This addresses extism/java-sdk#27

Both [as_ptr][as_ptr] and [as_mut_ptr][as_mut_ptr] are allowed
to return a dangling raw pointer when the Vec size is 0.

The idea is that you should guard that read checking the size.
This probably works well in most cases, but at the very least
in the java-sdk, the JNA machinery tries to be helpful and it
dereferences the pointer, causing a SIGSEGV.

The solution is to check if the resulting vector is empty
and return null instead. A new, empty vector would be better,
but I think that would not solve the problem, because
the problem is caused by a new, empty vector in the first
place.

Caveat: this might break consumers downstream.
On the other hand: consumers that do not check for the
nInput, nOutput counts are just waiting to explode, like
JNA.

[as_ptr]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.as_ptr
[as_mut_ptr]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.as_mut_ptr

Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi evacchi requested a review from zshipko August 30, 2024 17:40
evacchi added a commit to evacchi/java-sdk that referenced this pull request Aug 30, 2024
This addresses extism#27
once we merge extism/extism#760
and release a new version of libextism.

This PR simply check the parameters for null and the counts
to be valid, in preparation for extism/extism#760
which otherwise would cause a NullPointerException when
outputs and inputs are empty.

Signed-off-by: Edoardo Vacchi <[email protected]>
@evacchi
Copy link
Copy Markdown
Contributor Author

evacchi commented Aug 30, 2024

I am not sure what's wrong with CI 😅 WorksOnMyMachine™️

@evacchi evacchi changed the title Vec as_ptr() might return a dangling pointer fix: Vec.as_ptr() might return a dangling pointer Aug 30, 2024
evacchi added a commit to evacchi/java-sdk that referenced this pull request Sep 3, 2024
This addresses extism#27
once we merge extism/extism#760
and release a new version of libextism.

This PR simply check the parameters for null and the counts
to be valid, in preparation for extism/extism#760
which otherwise would cause a NullPointerException when
outputs and inputs are empty.

Signed-off-by: Edoardo Vacchi <[email protected]>
@zshipko zshipko merged commit 34096bd into extism:main Sep 4, 2024
@evacchi evacchi deleted the fix-dangling-ptr branch September 5, 2024 09:42
zshipko pushed a commit to extism/java-sdk that referenced this pull request Sep 5, 2024
This addresses #27 once we
merge extism/extism#760 and release a new
version of libextism.

This PR simply check the parameters for null and the counts to be valid,
in preparation for extism/extism#760 which
otherwise would cause a NullPointerException when outputs and inputs are
empty.

Signed-off-by: Edoardo Vacchi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants