Skip to content
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 invalid usage of reflect.SliceHeader #376

Merged
merged 1 commit into from
Aug 18, 2021
Merged

Conversation

cuonglm
Copy link
Contributor

@cuonglm cuonglm commented Aug 16, 2021

Using reflect.Sliceheader as plain struct violates the unsafe pointer
rule 6th.

@ti-mo
Copy link
Collaborator

ti-mo commented Aug 17, 2021

Hi @cuonglm, thanks for the patch! Could you leave the comments in place? Those still apply regardless of this fix.

@cuonglm
Copy link
Contributor Author

cuonglm commented Aug 18, 2021

Hi @cuonglm, thanks for the patch! Could you leave the comments in place? Those still apply regardless of this fix.

Hi @ti-mo, would you mind elaborating how can we solve this with unsafe.Slice?

AFAICT, unsafe.Slice need to work with pointer, while unsafe.Pointer does not have pointer semantic.

@ti-mo
Copy link
Collaborator

ti-mo commented Aug 18, 2021

Hi @cuonglm, I mostly meant the comment about removing support for marshaling unsafe.Pointers, that's something we should still consider.

I just tried playing around with unsafe.Slice a bit, and by the looks of it, it makes use of type params already? nvm, this just seems inherent to ArbitraryType. Actually, seems like unsafe.Slice might be a generic function after all. Its return type magically changes based on its input parameter. 🙂

The following seems to work and passes our current test suite:

	case unsafe.Pointer:
		dst := unsafe.Slice((*byte)(value), len(buf))
		copy(dst, buf)
		return nil

If I understand correctly, no longer need to call runtime.KeepAlive on value since we no longer take out a uintptr.

Until we drop support for pre-1.17, I would still go for your solution as the docs indeed prescribe that only existing slices must be reinterpreted using reflect.SliceHeader, and the caller should never allocate their own.

cc @lmb

Using reflect.Sliceheader as plain struct violates the unsafe pointer
rule 6th.
@cuonglm
Copy link
Contributor Author

cuonglm commented Aug 18, 2021

@ti-mo Thanks for clarification! I restored + updated the comment to make it a bit clearer.

@ti-mo ti-mo merged commit b557d8e into cilium:master Aug 18, 2021
@ti-mo
Copy link
Collaborator

ti-mo commented Aug 18, 2021

@cuonglm Thanks!

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.

None yet

2 participants