FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/


Description:
cgo: protect against zero divide when computing padding for zero-sized
objects
Fixes issue 4114.

Please review this at http://codereview.appspot.com/6553050/

Affected files:
M src/cmd/cgo/out.go


Index: src/cmd/cgo/out.go
===================================================================
--- a/src/cmd/cgo/out.go
+++ b/src/cmd/cgo/out.go
@@ -215,7 +215,7 @@
fmt.Fprint(&buf, "struct {\n")
off := int64(0)
for i, t := range n.FuncType.Params {
- if off%t.Align != 0 {
+ if t.Align !=0 && off%t.Align != 0 {
pad := t.Align - off%t.Align
fmt.Fprintf(&buf, "\t\tchar __pad%d[%d];\n", off, pad)
off += pad
@@ -233,7 +233,7 @@
off += pad
}
if t := n.FuncType.Result; t != nil {
- if off%t.Align != 0 {
+ if t.Align !=0 && off%t.Align != 0 {
pad := t.Align - off%t.Align
fmt.Fprintf(&buf, "\t\tchar __pad%d[%d];\n", off, pad)
off += pad
@@ -499,7 +499,7 @@
forFieldList(fntype.Params,
func(i int, atype ast.Expr) {
t := p.cgoType(atype)
- if off%t.Align != 0 {
+ if t.Align !=0 && off%t.Align != 0 {
pad := t.Align - off%t.Align
ctype += fmt.Sprintf("\t\tchar __pad%d[%d];\n", npad, pad)
off += pad
@@ -517,7 +517,7 @@
forFieldList(fntype.Results,
func(i int, atype ast.Expr) {
t := p.cgoType(atype)
- if off%t.Align != 0 {
+ if t.Align != 0 && off%t.Align != 0 {
pad := t.Align - off%t.Align
ctype += fmt.Sprintf("\t\tchar __pad%d[%d];\n", npad, pad)
off += pad

Search Discussions

  • Iant at Sep 21, 2012 at 2:12 pm
    https://codereview.appspot.com/6553050/diff/3001/src/cmd/cgo/out.go
    File src/cmd/cgo/out.go (right):

    https://codereview.appspot.com/6553050/diff/3001/src/cmd/cgo/out.go#newcode218
    src/cmd/cgo/out.go:218: if t.Align != 0 && off%t.Align != 0 {
    What if we instead change the "union" case in typeConv.Type in gcc.go to
    set t.Align = 1? It's not clear to me that t.Align == 0 should ever be
    valid.

    https://codereview.appspot.com/6553050/
  • Russ Cox at Sep 21, 2012 at 2:36 pm
    Align=1 sgtm
  • Rob Pike at Sep 21, 2012 at 7:30 pm
    Is there any other way for a zero alignment to arise in a C program?
    I'm not even sure why it arises for a union.

    -rob
  • Ian Lance Taylor at Sep 21, 2012 at 7:38 pm

    On Fri, Sep 21, 2012 at 12:30 PM, Rob Pike wrote:
    Is there any other way for a zero alignment to arise in a C program?
    I'm not even sure why it arises for a union.
    it doesn't--I think it's just a bug that cgo fails to set t.Align.

    Ian
  • R at Sep 21, 2012 at 7:30 pm
    Hello golang-dev@googlegroups.com, iant@golang.org, rsc@golang.org (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6553050/
  • Iant at Sep 21, 2012 at 7:41 pm
    https://codereview.appspot.com/6553050/diff/8001/src/cmd/cgo/gcc.go
    File src/cmd/cgo/gcc.go (right):

    https://codereview.appspot.com/6553050/diff/8001/src/cmd/cgo/gcc.go#newcode1203
    src/cmd/cgo/gcc.go:1203: t.Align = 1 // No useful alignment information
    for union fields.
    No need to split up these cases. They should both be handled the same
    way, and both should set t.Align = 1. We're setting t.Align = 1 because
    we're ignoring the contents of the class/union data and treating it as
    an array of byte. We could determine the correct alignment by looking
    at the fields, but why bother?

    https://codereview.appspot.com/6553050/
  • Devon H. O'Dell at Sep 21, 2012 at 7:43 pm
    2012/9/21 <iant@golang.org>:
    https://codereview.appspot.com/6553050/diff/8001/src/cmd/cgo/gcc.go
    File src/cmd/cgo/gcc.go (right):

    https://codereview.appspot.com/6553050/diff/8001/src/cmd/cgo/gcc.go#newcode1203
    src/cmd/cgo/gcc.go:1203: t.Align = 1 // No useful alignment information
    for union fields.
    No need to split up these cases. They should both be handled the same
    way, and both should set t.Align = 1. We're setting t.Align = 1 because
    we're ignoring the contents of the class/union data and treating it as
    an array of byte. We could determine the correct alignment by looking
    at the fields, but why bother?
    I'm fairly sure the C memory model specifies that unions are aligned
    to the constraints of their largest member, so it seems plausible that
    we could hit ABI issues (especially if the unions we use are for
    type-punning). I think we might need a func (*typeConv) Union(...) for
    this.

    --dho
  • R at Sep 21, 2012 at 7:47 pm
    Hello golang-dev@googlegroups.com, iant@golang.org, rsc@golang.org,
    iant@google.com, devon.odell@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    http://codereview.appspot.com/6553050/
  • Iant at Sep 21, 2012 at 7:48 pm
  • Devon Odell at Sep 21, 2012 at 7:49 pm
  • R at Sep 21, 2012 at 9:32 pm
    *** Submitted as
    http://code.google.com/p/go/source/detail?r=24d74bd32d81 ***

    cgo: set alignment to 1 for unions and classes; avoids crash from
    divide-by-zero
    Fixes issue 4114.

    R=golang-dev, iant, rsc, iant, devon.odell
    CC=golang-dev
    http://codereview.appspot.com/6553050


    http://codereview.appspot.com/6553050/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 21, '12 at 10:44a
activeSep 21, '12 at 9:32p
posts12
users5
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase