git2go: small fixes to odb module #447

2018-07-03T18:51:49-05:00

ghost commented

2018-07-03 18:51:49 -05:00

(Migrated from github.com)

Fix couple cgo issues in odb.Write() and odb.Hash(). This is the
same issue - and same solution - as repo.CreateBlobFromBuffer()
used to have.
Add test for odb.Read()

- Fix couple cgo issues in odb.Write() and odb.Hash(). This is the same issue - and same solution - as repo.CreateBlobFromBuffer() used to have. - Add test for odb.Read()

ghost commented

2018-07-04 19:56:41 -05:00

(Migrated from github.com)

I want to add - OdbReadStream.Read() and OdbWriteStream.Write() also look suspicious to me, but I wasn't able to write a test that triggers any issue that might be there...

ghost commented

2018-08-29 18:57:34 -05:00

(Migrated from github.com)

Ping - is there anything else I can do to unblock this ?

carlosmn commented

2019-01-02 16:57:39 -06:00

(Migrated from github.com)

What is the problem that CreateBlobFromBuffer used to have that this is meant to be fixing?

What is the problem that `CreateBlobFromBuffer` used to have that this is meant to be fixing?

ghost commented

2019-01-02 18:04:46 -06:00

(Migrated from github.com)

Hey Carlos,

CreateBlobFromBuffer() used to panic due to cgo issues when being fed bytes from a short bytes.Buffer{} - IIRC, bytes.Buffer{} allocates the first 64 bytes in place, and when cgo sees a pointer to these first 64 bytes, it thinks it is passed a pointer to a go object (the bytes.Buffer) that holds pointers to other go objects (the first 64 bytes) and panics.

The same issue is present in odb.Write() and odb.Hash(). The updated TestOdbHash() will trigger this cgo panic by using doublePointerBytes(), which uses the same construct as bytes.Buffer{}. The updated odb.Write() and odb.Hash() functions will work again without triggering the cgo issue anymore.

The changes to TestOdbRead() are not 100% necessary, but since I changed odb.Write() I wanted to increase its test coverage while I was there.

I also had cgo maintainer Ian Taylor check the changes for me at work - I would not normally resort to this kind of argument by authority, but low-level cgo stuff is obscure enough that I wouldn't be as confident about the changes if he hadn't, so probably worth adding here.

Hey Carlos, CreateBlobFromBuffer() used to panic due to cgo issues when being fed bytes from a short bytes.Buffer{} - IIRC, bytes.Buffer{} allocates the first 64 bytes in place, and when cgo sees a pointer to these first 64 bytes, it thinks it is passed a pointer to a go object (the bytes.Buffer) that holds pointers to other go objects (the first 64 bytes) and panics. The same issue is present in odb.Write() and odb.Hash(). The updated TestOdbHash() will trigger this cgo panic by using doublePointerBytes(), which uses the same construct as bytes.Buffer{}. The updated odb.Write() and odb.Hash() functions will work again without triggering the cgo issue anymore. The changes to TestOdbRead() are not 100% necessary, but since I changed odb.Write() I wanted to increase its test coverage while I was there. I also had cgo maintainer Ian Taylor check the changes for me at work - I would not normally resort to this kind of argument by authority, but low-level cgo stuff is obscure enough that I wouldn't be as confident about the changes if he hadn't, so probably worth adding here.

carlosmn commented

2019-01-03 09:22:09 -06:00

(Migrated from github.com)

I see, tanks for the explanation.

Sign in to join this conversation.

No reviewers