linter: ignore false positives in types/address

[robert-zaremba]

Description

Recent linter update created new issues in master. This solves issues in types/address - which in fact are false positives.

closes: #8670

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[codecov]

Codecov Report

Merging #8674 (52a2373) into master (f2ee972) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8674      +/-   ##
==========================================
- Coverage   61.47%   61.47%   -0.01%     
==========================================
  Files         659      659              
  Lines       37916    37916              
==========================================
- Hits        23310    23308       -2     
- Misses      12167    12168       +1     
- Partials     2439     2440       +1     

Impacted Files	Coverage Δ
types/address/hash.go	100.00% <100.00%> (ø)
snapshots/store.go	74.31% <0.00%> (-1.10%)	⬇️

[odeke-em]

Thanks for the change. However the vet violations are legitimate reports per the unsafe rules https://golang.org/pkg/unsafe/:
a) You should only use reflect.SliceHeader or reflect.StringHeader as pointers and not as plain structs

it should be sh := (*reflect.SliceHeader)(unsafe.Pointer(&s)) and not sh := *(*reflect.SliceHeader)(unsafe.Pointer(&s))

b) You shouldn't store a value cast from unsafe.Pointer, and should just return it, otherwise you could need a runtime.KeepAlive
it should be *(*[]byte)(unsafe.Pointer(sh)) and not

        bs := *(*[]byte)(unsafe.Pointer(&sh))
        return bs

Which therefore the correct version should be

func conv(s string) []byte {
        sh := (*reflect.SliceHeader)(unsafe.Pointer(&s))
        sh.Cap = sh.Len
        return *(*[]byte)(unsafe.Pointer(sh))
}

I'd suggest, let's rollback that ignore, and then fix it properly for safety.

/cc @alessio

[robert-zaremba]

Thank you @odeke-em for verifying it. Let me double check here the correctness. The reason I used (*reflect.SliceHeader)(unsafe.Pointer(&s)) was to create a new Header. Slice (SliceHeader) doesn't have Cap - so it is a smaller structure. So if we write there by casting a pointer, we are potentially overwriting some other structure memory. So, I'm not convinced that the solution above is correct - we rather need to create a new Header.

I'm not sure if GC will eat the string since in my solution we ware copying it in the same context. However, the following solution (from stackoverflow) should should solve our concerns:

var buf = *(*[]byte)(unsafe.Pointer(&str))   // here we copy the header in a managed way
(*reflect.SliceHeader)(unsafe.Pointer(&buf)).Cap = len(str)

[odeke-em]

You can definitely create a new SliceHeader but keep it as a pointer, otherwise at that point you violate the rules. Yup the Capacity copy is valid and in the suggestion I made it still exists. The second version from Stackoverflow works, but you haven’t changed much, modifying the returned byte slice will modify the prior string contents so you’ll be back at the problem you were trying to solve, and it isn’t any clearer, I’d go for clarity yet correctness than more mangling. Good stuff further researching!

…

On Wed, Feb 24, 2021 at 3:15 AM Robert Zaremba ***@***.***> wrote: Thank you @odeke-em <https://github.com/odeke-em> for verifying it. Let me double check here the correctness. The reason I used (*reflect.SliceHeader)(unsafe.Pointer(&s)) was to create a new Header. Slice (SliceHeader) doesn't have Cap - so it is a smaller structure. So if we write there by casting a pointer, we are potentially overwriting some other structure memory. So, I'm not convinced that the solution above is correct - we rather need to create a new Header. I'm not sure if GC will eat the string since in my solution we ware copying it in the same context. However, the following solution (from stackoverflow <https://stackoverflow.com/a/66218124/297094>) should should solve our concerns: var buf = *(*[]byte)(unsafe.Pointer(&str)) // here we copy the header in a managed way (*reflect.SliceHeader)(unsafe.Pointer(&buf)).Cap = len(str) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8674 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFL3V6SWDJ7MBEWOZBVZ5TTATNUVANCNFSM4YCPCDGA> .

[robert-zaremba]

Thanks again for insights.
I want to copy the header in order to safely modify the Cap.

modifying the returned byte slice will modify the prior string contents so you’ll be back at the problem you were trying to solve,

The goal is to change the structure kind for usage we control - here it is read only for reading bytes for when marshaling the content. So there is no other modification of the underlying array. Am I missing something @odeke-em ?

[odeke-em]

Great that your use case is to not modify the returned byte slice. I added a suggestion to place a red hot warning that callers of the new utility function should never modify the byte slice. After that you are all gucci to go :-) Thank you.