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

runtime/cgo: C.CString causes index out of range when parameter is too long #53958

Closed
hoyhbx opened this issue Jul 19, 2022 · 6 comments
Closed
Assignees
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@hoyhbx
Copy link

hoyhbx commented Jul 19, 2022

What version of Go are you using (go version)?

$ go version
go version go1.18.3 linux/amd64

Does this issue reproduce with the latest release?

Not sure, only tested on go1.18.3 linux/amd64

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/home/hoyhbx/.cache/go-build"
GOENV="/home/hoyhbx/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/home/hoyhbx/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/home/hoyhbx/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org/,direct"
GOROOT="/home/hoyhbx/.local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/home/hoyhbx/.local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.18.3"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/dev/null"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build756945598=/tmp/go-build -gno-record-gcc-switches"

What did you do?

We tried to build a go function that utilizes a k8s function to parse quantites. Then, we wanted to call the go function we built from C code to test whether it would work. However, we found that when the number we parsed had a very large absolute value (e.g. -92743e6047801799), the go function crashed with index out of range. The index out of range happens on the line return C.CString(q.AsDec().String()), we suspect this is caused because the parameter of the C.CString is too long.

reproduce.zip

The Go file and C file we created are as follows. Note that k8sutil.h we used was generated by cmd/cgo.

Go file
package main

import "C"
import (
	"fmt"
	"k8s.io/apimachinery/pkg/api/resource"
)

//export parse
func parse(valuePtr *C.char) *C.char {
	value := C.GoString(valuePtr)
	q, err := resource.ParseQuantity(value)
	if err != nil {
		return C.CString("INVALID")
	}
	return C.CString(q.AsDec().String())
}

func main() {
	fmt.Println("Hello, world.")
}
C file
#include <stdio.h>
#include "k8sutil.h"

int main(){
    char *name = "-92743e6047801799";
    printf(parse(name));
    return 0;
}

We compiled the code by running the following commands:

go build -buildmode=c-shared -o k8sutil.so k8sutil.go
gcc test.c -o test ./k8sutil.so

Then, we ran the program by running ./test.

What did you expect to see?

We expected to see the long string gets returned correctly and the program does not crash.

What did you see instead?

The program crashed with the following error:

panic: runtime error: index out of range [1752834508] with length 1073741824

goroutine 17 [running, locked to thread]:
main._Cfunc_CString({0xc000680000, 0x687a21cc})
        _cgo_gotypes.go:49 +0xab
main.parse(0x0?)
        /home/hoyhbx/k8sutil.go:18 +0xba
Aborted (core dumped)
@hoyhbx hoyhbx changed the title affected/package: ago runtime/cgo: C.CString causes index out of range when parameter is too long Jul 19, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Jul 19, 2022
@randall77
Copy link
Contributor

The string is being passed back and forth correctly to Go. The only problem is that the returned string is >1GB in size, which is beyond the limit for C.CString.
The limit in C.CString is kind of arbitrary, it could be raised (or even eliminated) if needed. But is it really intended that this return a 1.7GB string?

@randall77
Copy link
Contributor

From https://pkg.go.dev/k8s.io/apimachinery/pkg/api/resource

No matter which of the three exponent forms is used, no quantity may represent a number greater than 2^63-1 in magnitude

It looks like you're well above that. Maybe ParseQuantity should be reporting an error?

@randall77
Copy link
Contributor

Looks like ParseQuantity saturates the result at 1<<63-1, but only for binary exponents, not base 10 exponents. Not sure why that is, but it is weird and maybe a bug.

https://github.com/kubernetes/apimachinery/blob/97e5df2d0258ac077093867fabf508c1fc31f53b/pkg/api/resource/quantity.go#L363

@toothrot toothrot added the NeedsFix The path to resolution is known, but the work has not been done. label Jul 20, 2022
@toothrot toothrot added this to the Backlog milestone Jul 20, 2022
@hoyhbx
Copy link
Author

hoyhbx commented Jul 20, 2022

Hi, @randall77 , thanks for your reply. Yeah the returned string is extremely large. But since C.CString is a primitive for the cgo runtime, we didn't expect it to fail.

BTW, what should the users do to raise the limit of CString?

@randall77
Copy link
Contributor

You can't. It's hardcoded at the moment. I'm working on a fix to remove the limit.

@gopherbot
Copy link

Change https://go.dev/cl/418557 mentions this issue: cmd/cgo: allow cgo to pass strings or []bytes bigger than 1<<30

@randall77 randall77 self-assigned this Jul 20, 2022
@dmitshur dmitshur modified the milestones: Backlog, Go1.19 Aug 1, 2022
jproberts pushed a commit to jproberts/go that referenced this issue Aug 10, 2022
There's no real reason to limit to 1<<30 bytes. Maybe it would catch
some mistakes, but probably ones that would quickly manifest in other
ways.

We can't use the fancy new unsafe.Slice function because this code
may still be generated for people with 1.16 or earlier in their go.mod file.
Use unsafe shenanigans instead.

Fixes golang#53965
Fixes golang#53958

Change-Id: Ibfa095192f50276091d6c2532e8ccd7832b57ca8
Reviewed-on: https://go-review.googlesource.com/c/go/+/418557
Run-TryBot: Keith Randall <khr@golang.org>
Reviewed-by: Ian Lance Taylor <iant@google.com>
Reviewed-by: Keith Randall <khr@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@golang golang locked and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

5 participants