8 ms·

Fix for Golang runtime memory corruption bug

115 points by rargulati 10y ago

rargulati 10y ago
An alternative view: https://github.com/golang/go/commit/b902a63ade47cf69218c9b38c9a783fc8cfc3041 https://github.com/golang/go/commit/b902a63ade47cf69218c9b38...
deleted 10y ago
[deleted]
morecoffee 10y ago
One thing I like about rsc (and Austin) is that they have these well explained commit logs that let's people follow along at home. The changes seem like they are well justified and made for posterity. This change in particular is a story more than it is a 3 LOC fix.
- idank 10y ago
  You weren't joking, it is a story. Isn't it kind of unfortunate that all this information is in a commit message, that 1) needs to be discovered through git blame, 2) at risk of requiring code archeology if enough lines change, e.g. by moving code around, and 3) is immutable so parts of it cannot be edited in the future? I see this habit in a lot of commit messages and while it is important to convey intent, there are better ways to document your code (either inline, or in this particular case, a wiki page).
  - obstinate 10y ago
    Much of it is reproduced in the comment in the patch.
    - LukeShu 10y ago
      Indeed, the only actual code change is: - sellock(scases, lockorder) + systemstack(func() { + sellock(scases, lockorder) + }) The rest of the patch is adding/changing comments explaining everything.
      - bsdetector 10y ago
        I feel this kind of thing should be remembered when people say they're going to reinvent decades-old technology. This bug exists because they wanted to reinvent threading so you could have millions of threads, except when you actually use millions of threads for anything non-trivial it doesn't actually work out so well. To do that they couldn't use the standard since-1970 fixed stacks so they needed movable/growable stacks. They reinvented exceptions in a way that broke selinux. They reinvented the memory allocator in a way that breaks ulimit. They reinvented a javadoc/doxygen that's less useful. It's great to try to improve things, but a lot of times there's actually very good reasons for the old ways.
        rounce 10y ago
        Are you kidding? How is `go doc` anything like Javadoc aside from being automatically generated by machine from your sources. Might as well compare it to Jekyll.
        gravypod 10y ago
        https://docs.oracle.com/javase/8/docs/api/java/util/List.html#size-- https://docs.oracle.com/javase/8/docs/api/java/util/List.htm... https://golang.org/pkg/container/list/#List.Len https://golang.org/pkg/container/list/#List.Len
        rounce 10y ago
        Note the backticks, I was referring to the `go doc` command built into the standard tooling. There are many generated HTML documentation generation tools for every language. It's Golang's general approach to documentation which many find appealing, not the fact they can output to HTML.
        gravypod 10y ago
        My tooling does this too and I don't even need to remember a terminal command: http://i.imgur.com/YmAkBPG.png http://i.imgur.com/YmAkBPG.png I get what your saying but it's apples to oranges. They are in effect similar though.
- masklinn 10y ago
  I like going through Postgres's commit logs once in a while for exactly that reason, non-trivial commits (logic wise, they may be a 1LOC commit) get short-story-scale messages, often with references to ML thread(s) where the issue or change was discussed. It's a nice change of pace from $dayjob where a commit changing half the codebase in ways unspecified gets logged as "improvements".
  - koolba 10y ago
    > It's a nice change of pace from $dayjob where a commit changing half the codebase in ways unspecified gets logged as "improvements". The best (worst?) one is where you'll see a series of major changes with the following commit messages: - Add feature X (no explanation of what it does) - Fix feature X - Really fix feature X - Fix feature X for real - Finally fixed X - Fix test environment
    - masklinn 10y ago
      I know you don't work at $dayjob because the last commit would be "fix X test which is dumb", which removes the test entirely.
0x0 10y ago
Does this mean every single application binary compiled with go in the wild up until today has a memory corruption bug? If so, it's going to take some effort for this bug to be weeded out in every single deployment around the world...
- msiebuhr 10y ago
  The bug was introduced in Go 1.5 (when the compiler was rewritten to Go)
  - mseepgood 10y ago
    No, it was introduced in Go 1.4, when the runtime was rewritten in Go.
- deleted 10y ago
  [deleted]
- Thaxll 10y ago
  I don't understand this comment, are you implying that it's the first time you see a runtime with a bug?
  - 0x0 10y ago
    Well usually you can just update whatever library has the bug and restart your applications, but as I understand this bug will be statically linked in all application binaries so they will all have to be recompiled and redeployed?
    - Manishearth 10y ago
      Yes, that's what the grandparent is saying, this is a bug in the runtime, which you can't fix by updating libraries, you have to update the compiler/interpreter (depending on the language). This is not a unique occurrence.
      - 10y ago
akavel 10y ago
I wonder if it'll get backported to at least Go 1.7?
- pkroll 10y ago
  Not sure, but I'd expect it to get fixed in 1.4, since that's the version needed to generate later versions if you're bootstrapping.
millstone 10y ago
I'm surprised there was no test added to ensure this doesn't regress. Any reason why not?
- more_original 10y ago
  Looks like it's a concurrency bug that is only triggered non-deterministically under certain circumstances. It is probably hard to reproduce reliably in a test, if this is possible at all.
loppers92 10y ago
Some of the points are just ridiculous. Because you can't compare java and go or whatever. If you want complain about a language you should first checking the history of java or whatever. Maybe this language has runtime bugs in old versions as well.
johansch 10y ago
Somewhat OT: Just having recently gotten into Go: How about changing the official name from Go to Golang? It's not ideal, but so wasn't the naming in the first place :/ Go isn't particularly search friendly. Quite ironic for a language coming out of the world's largest search company. Just try searching for go in the otherwise excellent Algolia HN search engine: Without quotes: http://i.imgur.com/8ztH48u.png http://i.imgur.com/8ztH48u.png With quotes: http://i.imgur.com/5a229fC.png http://i.imgur.com/5a229fC.png

115 points by rargulati 10y ago

An alternative view: https://github.com/golang/go/commit/b902a63ade47cf69218c9b38c9a783fc8cfc3041 https://github.com/golang/go/commit/b902a63ade47cf69218c9b38...

deleted 10y ago

[deleted]

morecoffee 10y ago

One thing I like about rsc (and Austin) is that they have these well explained commit logs that let's people follow along at home. The changes seem like they are well justified and made for posterity. This change in particular is a story more than it is a 3 LOC fix.

idank 10y ago
You weren't joking, it is a story. Isn't it kind of unfortunate that all this information is in a commit message, that 1) needs to be discovered through git blame, 2) at risk of requiring code archeology if enough lines change, e.g. by moving code around, and 3) is immutable so parts of it cannot be edited in the future? I see this habit in a lot of commit messages and while it is important to convey intent, there are better ways to document your code (either inline, or in this particular case, a wiki page).
- obstinate 10y ago
  Much of it is reproduced in the comment in the patch.
  - LukeShu 10y ago
    Indeed, the only actual code change is: - sellock(scases, lockorder) + systemstack(func() { + sellock(scases, lockorder) + }) The rest of the patch is adding/changing comments explaining everything.
    - bsdetector 10y ago
      I feel this kind of thing should be remembered when people say they're going to reinvent decades-old technology. This bug exists because they wanted to reinvent threading so you could have millions of threads, except when you actually use millions of threads for anything non-trivial it doesn't actually work out so well. To do that they couldn't use the standard since-1970 fixed stacks so they needed movable/growable stacks. They reinvented exceptions in a way that broke selinux. They reinvented the memory allocator in a way that breaks ulimit. They reinvented a javadoc/doxygen that's less useful. It's great to try to improve things, but a lot of times there's actually very good reasons for the old ways.
      - rounce 10y ago
        Are you kidding? How is `go doc` anything like Javadoc aside from being automatically generated by machine from your sources. Might as well compare it to Jekyll.
        gravypod 10y ago
        https://docs.oracle.com/javase/8/docs/api/java/util/List.html#size-- https://docs.oracle.com/javase/8/docs/api/java/util/List.htm... https://golang.org/pkg/container/list/#List.Len https://golang.org/pkg/container/list/#List.Len
        rounce 10y ago
        Note the backticks, I was referring to the `go doc` command built into the standard tooling. There are many generated HTML documentation generation tools for every language. It's Golang's general approach to documentation which many find appealing, not the fact they can output to HTML.
        gravypod 10y ago
        My tooling does this too and I don't even need to remember a terminal command: http://i.imgur.com/YmAkBPG.png http://i.imgur.com/YmAkBPG.png I get what your saying but it's apples to oranges. They are in effect similar though.
masklinn 10y ago
I like going through Postgres's commit logs once in a while for exactly that reason, non-trivial commits (logic wise, they may be a 1LOC commit) get short-story-scale messages, often with references to ML thread(s) where the issue or change was discussed. It's a nice change of pace from $dayjob where a commit changing half the codebase in ways unspecified gets logged as "improvements".
- koolba 10y ago
  > It's a nice change of pace from $dayjob where a commit changing half the codebase in ways unspecified gets logged as "improvements". The best (worst?) one is where you'll see a series of major changes with the following commit messages: - Add feature X (no explanation of what it does) - Fix feature X - Really fix feature X - Fix feature X for real - Finally fixed X - Fix test environment
  - masklinn 10y ago
    I know you don't work at $dayjob because the last commit would be "fix X test which is dumb", which removes the test entirely.

0x0 10y ago

Does this mean every single application binary compiled with go in the wild up until today has a memory corruption bug? If so, it's going to take some effort for this bug to be weeded out in every single deployment around the world...

msiebuhr 10y ago
The bug was introduced in Go 1.5 (when the compiler was rewritten to Go)
- mseepgood 10y ago
  No, it was introduced in Go 1.4, when the runtime was rewritten in Go.
deleted 10y ago
[deleted]
Thaxll 10y ago
I don't understand this comment, are you implying that it's the first time you see a runtime with a bug?
- 0x0 10y ago
  Well usually you can just update whatever library has the bug and restart your applications, but as I understand this bug will be statically linked in all application binaries so they will all have to be recompiled and redeployed?
  - Manishearth 10y ago
    Yes, that's what the grandparent is saying, this is a bug in the runtime, which you can't fix by updating libraries, you have to update the compiler/interpreter (depending on the language). This is not a unique occurrence.
    - 10y ago

akavel 10y ago

I wonder if it'll get backported to at least Go 1.7?

pkroll 10y ago
Not sure, but I'd expect it to get fixed in 1.4, since that's the version needed to generate later versions if you're bootstrapping.

millstone 10y ago

I'm surprised there was no test added to ensure this doesn't regress. Any reason why not?

more_original 10y ago
Looks like it's a concurrency bug that is only triggered non-deterministically under certain circumstances. It is probably hard to reproduce reliably in a test, if this is possible at all.

loppers92 10y ago

Some of the points are just ridiculous. Because you can't compare java and go or whatever. If you want complain about a language you should first checking the history of java or whatever. Maybe this language has runtime bugs in old versions as well.

johansch 10y ago

Somewhat OT: Just having recently gotten into Go: How about changing the official name from Go to Golang? It's not ideal, but so wasn't the naming in the first place :/ Go isn't particularly search friendly. Quite ironic for a language coming out of the world's largest search company. Just try searching for go in the otherwise excellent Algolia HN search engine: Without quotes: http://i.imgur.com/8ztH48u.png http://i.imgur.com/8ztH48u.png With quotes: http://i.imgur.com/5a229fC.png http://i.imgur.com/5a229fC.png