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

Slice of runes doesn't trigger "out of bounds" panic #14232

Closed
VojtechVitek opened this issue Feb 4, 2016 · 14 comments
Closed

Slice of runes doesn't trigger "out of bounds" panic #14232

VojtechVitek opened this issue Feb 4, 2016 · 14 comments

Comments

@VojtechVitek
Copy link

package main

import "fmt"

func main() {
    s := "ž"
    fmt.Println(string([]rune(s)[1:10])) // doesn't panic, prints 桉��𲍀桉P�𲇀桉
    fmt.Println([]rune(s)[1:10]) // panics
}

http://play.golang.org/p/RHdDVEdo7H

I'm not sure if this is bug -- but I'd expect panic on both lines. Why the string()-encapsulated slice doesn't trigger the panic?

@c2h5oh
Copy link

c2h5oh commented Feb 4, 2016

Something even more scary:

package main

import (
    "fmt"
)

func main() { 
        s := "ž" 

    fmt.Println(string([]rune(s)[0:32])) 

    fmt.Println()

    fmt.Printf("%x\n",string([]rune(s)[0:32]))
    fmt.Printf("%x\n",string([]rune(s)[0:32]))
    fmt.Printf("%x\n",string([]rune(s)[0:32]))
    fmt.Printf("%x\n",string([]rune(s)[0:32]))

    fmt.Println()   


    for i := 1; i <10; i++ {
        fmt.Printf("%x\n",string([]rune(s)[0:32]))
    }
} 

Prints:

ž�����𲰠�󎧠竵��𲶠竵P�𲰠竵��竵��󂱠竵�竵𢗠竵

c5bee7abb5efbfbdefbfbdefbfbdefbfbdefbfbde7abb500211f02efbfbd02c78902f0b29f80e7abb5efbfbd02f0b29780e7abb5efbfbd0808efbfbdefbfbdefbfbd41efbfbd00efbfbd
c5bef0b9a2a0f0b9a2a0e7abb500efbfbd08efbfbd00efbfbdf0ba93a0efbfbdefbfbdc290efbfbdefbfbd20232300f0a297a0e7abb5f0b092a0e7abb5efbfbd0100f28385a0f0a29881e7abb5efbfbdefbfbd
c5bee7abb5efbfbdefbfbd1cefbfbd00efbfbdefbfbdefbfbdefbfbdefbfbd23efbfbd00efbfbdf0b29780efbfbdefbfbd02f0a298a002f1a28a80e7abb5efbfbd02f0b29780e7abb5efbfbd0808efbfbd
c5be00efbfbd00f0b2978000000708000058efbfbdefbfbdefbfbd00efbfbd00efbfbd00efbfbdefbfbdefbfbd00f0b29780e7abb5efbfbd201cefbfbd0008

c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00
c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00
c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00
c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00
c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00
c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00
c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00
c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00
c5bee7abb5efbfbd00efbfbd000100efbfbd00efbfbde7abb503efbfbd00efbfbd7700efbfbd18180018efbfbdefbfbd00efbfbd00efbfbd00efbfbd00

http://play.golang.org/p/rym8a5JpEO

The output in the loop is the same for all iterations, but that's not true for individual prints suggesting at least something in the area of the rune leaks.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2016

Yeah, this is buggy.

http://play.golang.org/p/gma2M0wVxO

It does do a bounds check if the slice is on a variable instead of a conversion expression:

http://play.golang.org/p/KH3KD6MWrw

/cc @rsc @griesemer

@bradfitz bradfitz added this to the Go1.6Maybe milestone Feb 4, 2016
@dominikh
Copy link
Member

dominikh commented Feb 4, 2016

And panics for sufficiently large indices: http://play.golang.org/p/xb-ulwT1b0

@kardianos
Copy link
Contributor

The slice array []rune(s) has a cap of 32. This is perfectly legal and safe as far as I can tell.

Sorry, yes, I retract this. Agree there is a problem.

@c2h5oh
Copy link

c2h5oh commented Feb 4, 2016

@kardianos see prints that are not in a loop - it's printing seemingly random data beyond the 2 bytes used by the actual rune content - possible out of bounds read.

@dominikh
Copy link
Member

dominikh commented Feb 4, 2016

http://play.golang.org/p/j9_oAeLzuu different capacity of the rune slice, depending on how it's used later on.

@randall77
Copy link
Contributor

I believe the only bug here is that the conversion from string to []rune doesn't zero the data between len and cap.
The code is in src/runtime/string.go. It uses rawruneslice to allocate the slice, and only overwrites up to len. rawruneslice (and probably rawbyteslice) should zero from len up to the cap.

@bradfitz
Copy link
Contributor

bradfitz commented Feb 4, 2016

/cc @dr2chase @randall77 too

@dominikh
Copy link
Member

dominikh commented Feb 4, 2016

@randall77 should a type conversion like string -> rune really return cap > len?

@randall77
Copy link
Contributor

Looks like rawruneslice and rawbyteslice do the right thing. The bug is when the compiler passes in a stack-allocated buffer for stringtoslicerune to fill. I'll code up a fix.

@ianlancetaylor
Copy link
Contributor

I note that this was also broken in Go 1.5, although 1.4 seems to get it right.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/19231 mentions this issue.

@randall77
Copy link
Contributor

@ianlancetaylor Yes, if we ever do a 1.5.4 we should include a fix for this.
It seems at least a mild security bug because it lets you read a portion of the stack frame that used to be at the same location as the current frame.

@dominikh It seems ok, even desirable, to return a slice with cap>len. If the backing slice is malloc'd, the unused space between the slice length and the sizeclass size is wasted, might as well let the caller allocate into it. I imagine a ([]byte)("...") followed by an append is reasonably common. Of course, as we see, it is a land of potential bugs...

@rsc
Copy link
Contributor

rsc commented Feb 5, 2016

I think the fix here was the right one. The cap should match the len, just as []byte{'a', 'b', 'c'} does. This was just a bug.

I don't believe there's any need for a Go 1.5.4.

@golang golang locked and limited conversation to collaborators Feb 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants