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.

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

Remove unnecessary memory copies by C.GoBytes() calls #154

Merged
merged 1 commit into from
Jan 26, 2022
Merged

Remove unnecessary memory copies by C.GoBytes() calls #154

merged 1 commit into from
Jan 26, 2022

Conversation

ansiwen
Copy link
Contributor

@ansiwen ansiwen commented Jan 17, 2022

It is not necessary to copy the memory in order to get a []byte
representation of Go allocated memory, because the GC will take care
of the lifetime of the underlying memory. This change introduces the
memBytes() function, that returns a slice to an arbitrary memory
area, and replaces all uses of C.GoBytes with Go allocated memory.
This function could even be used for C allocated memory, but then the
caller has to make sure that the underlying C memory is not freed
during the lifetime of the returned slice.

Signed-off-by: Sven Anderson [email protected]

@ansiwen
Remove unnecessary memory copies by C.GoBytes() calls
02b3205 
It is not necessary to copy the memory in order to get a []byte
representation of Go allocated memory, because the GC will take care
of the lifetime of the underlying memory.  This change introduces the
memBytes() function, that returns a slice to an arbitrary memory
area, and replaces all uses of C.GoBytes with Go allocated memory.
This function could even be used for C allocated memory, but then the
caller has to make sure that the underlying C memory is not freed
during the lifetime of the returned slice.

Signed-off-by: Sven Anderson <[email protected]>
miekg
miekg reviewed Jan 18, 2022
View reviewed changes
params.go
@@ -84,7 +84,7 @@ func cGCMParams(p *GCMParams) []byte {
p.Free()
p.arena = arena
p.params = &params
return C.GoBytes(unsafe.Pointer(&params), C.int(unsafe.Sizeof(params)))
return memBytes(unsafe.Pointer(&params), unsafe.Sizeof(params))
Copy link
Owner

Choose a reason for hiding this comment

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

But this memory isn't allocated by the Go runtime? So how/why can we let it clean it up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The params struct is allocated by the Go runtime in line 69. The only C memory that is allocated here is the memory pointed to by the pIv and pAAD pointers, which are owned by the arena list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is the confusion: While C.CString() and C.CBytes() both allocate C memory, C.GoString() and C.GoBytes() don't free any C memory. If that would be the case, it would crash before, because it would try to free Go allocated memory. All four functions do copy the data, though. So my change only skips the copy of the Go allocated memory. It does not change anything else.

Copy link
Owner

Choose a reason for hiding this comment

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

ah, thanks. This may/might also impact #103

Are you able to test on ARM? I've got severals PIs here, but none of the is operational

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My Pi is in use. But there should be ARM containers/VMs available... Let me check...

Copy link
Contributor Author

@ansiwen ansiwen Jan 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I just ran the tests in Docker for Mac which includes QEMU Arm Emulation:

ARM Test 
~/pkcs11 # go test -v
=== RUN   TestConstCouunt
--- PASS: TestConstCouunt (0.09s)
=== RUN   TestPSSParams
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
--- PASS: TestPSSParams (0.52s)
=== RUN   TestOAEPParams
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
--- PASS: TestOAEPParams (0.54s)
=== RUN   TestGCMParams
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
--- PASS: TestGCMParams (0.03s)
=== RUN   TestSetenv
--- PASS: TestSetenv (0.00s)
=== RUN   TestInitialize
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
--- PASS: TestInitialize (0.00s)
=== RUN   TestNew
--- PASS: TestNew (0.00s)
=== RUN   TestGetInfo
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
    pkcs11_test.go:110: {CryptokiVersion:{Major:2 Minor:40} ManufacturerID:SoftHSM Flags:0 LibraryDescription:Implementation of PKCS11 LibraryVersion:{Major:2 Minor:6}}
--- PASS: TestGetInfo (0.02s)
=== RUN   TestFindObject
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
--- PASS: TestFindObject (0.38s)
=== RUN   TestGetAttributeValue
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
    pkcs11_test.go:157: attr 0, type 290, valuelen 3
    pkcs11_test.go:157: attr 1, type 289, valuelen 8
    pkcs11_test.go:157: attr 2, type 288, valuelen 256
    pkcs11_test.go:161: modulus 23177777739565763036282288451612277215094384267580649141662939060143653520262815874118203995592856707364133701746712231446013495003062418685260033536897721621719332973876868208594342698847009386797801425213144004540616330415527202157984084012213855606210350141680422980461205958516644774066799652962912949996909837145873324004715098082409084003064991158954258350469636813350730092911010114461081646533062309803387303320248286189186258025689039259133805988684837466194122767775234756766518600432835863922488209702742743436552216975317554938048854637024088066334038385870014335695062757114883232386654747089000511233519
    pkcs11_test.go:157: attr 3, type 3, valuelen 17
--- PASS: TestGetAttributeValue (0.23s)
=== RUN   TestDigest
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
=== RUN   TestDigest/Simple
=== RUN   TestDigest/Empty
--- PASS: TestDigest (0.02s)
    --- PASS: TestDigest/Simple (0.00s)
    --- PASS: TestDigest/Empty (0.00s)
=== RUN   TestDigestUpdate
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
=== RUN   TestDigestUpdate/Simple
=== RUN   TestDigestUpdate/Empty
--- PASS: TestDigestUpdate (0.01s)
    --- PASS: TestDigestUpdate/Simple (0.00s)
    --- PASS: TestDigestUpdate/Empty (0.00s)
=== RUN   TestGenerateKeyPair
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
--- PASS: TestGenerateKeyPair (0.47s)
=== RUN   TestSign
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
--- PASS: TestSign (0.92s)
=== RUN   TestDestroyObject
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
--- PASS: TestDestroyObject (0.41s)
=== RUN   TestSymmetricEncryption
    pkcs11_test.go:33: loading /usr/lib/softhsm/libsofthsm2.so
=== RUN   TestSymmetricEncryption/Encrypt
=== RUN   TestSymmetricEncryption/Encrypt/ECB
=== RUN   TestSymmetricEncryption/Encrypt/CBC
=== RUN   TestSymmetricEncryption/Encrypt/CBC-PAD
=== RUN   TestSymmetricEncryption/EncryptUpdate
=== RUN   TestSymmetricEncryption/EncryptUpdate/ECB
=== RUN   TestSymmetricEncryption/EncryptUpdate/CBC
=== RUN   TestSymmetricEncryption/EncryptUpdate/CBC-PAD
--- PASS: TestSymmetricEncryption (0.04s)
    --- PASS: TestSymmetricEncryption/Encrypt (0.01s)
        --- PASS: TestSymmetricEncryption/Encrypt/ECB (0.00s)
        --- PASS: TestSymmetricEncryption/Encrypt/CBC (0.00s)
        --- PASS: TestSymmetricEncryption/Encrypt/CBC-PAD (0.00s)
    --- PASS: TestSymmetricEncryption/EncryptUpdate (0.01s)
        --- PASS: TestSymmetricEncryption/EncryptUpdate/ECB (0.01s)
        --- PASS: TestSymmetricEncryption/EncryptUpdate/CBC (0.00s)
        --- PASS: TestSymmetricEncryption/EncryptUpdate/CBC-PAD (0.00s)
=== RUN   ExampleCtx_Sign
--- PASS: ExampleCtx_Sign (0.37s)
PASS
ok  	github.com/miekg/pkcs11	4.188s
~/pkcs11 # go version
go version go1.17.4 linux/arm64
~/pkcs11 # git log --oneline | head -1
02b3205 Remove unnecessary memory copies by C.GoBytes() calls
~/pkcs11 #

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure it's related to #103. The C.free(unsafe.Pointer(sig)) looks legit to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To double check, I just ran the tests of my PR on an AWS arm64 instance, same results.

@ansiwen ansiwen requested a review from miekg January 23, 2022 23:47
@miekg
Copy link
Owner

miekg commented Jan 26, 2022

thanks for the checking!

I think this is OK to merge.

@miekg miekg merged commit 9a05b23 into miekg:master Jan 26, 2022
@ansiwen ansiwen deleted the pr/ansiwen/membytes branch February 3, 2022 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miekg miekg Awaiting requested review from miekg

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
@ansiwen @miekg