-
Notifications
You must be signed in to change notification settings - Fork 0
Expand file tree
/
Copy pathgovanish.html
More file actions
183 lines (183 loc) · 22.2 KB
/
Copy pathgovanish.html
File metadata and controls
183 lines (183 loc) · 22.2 KB
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="utf-8"/>
<meta name="author" content="Nikita Sivukhin"/>
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Go can remove code from the binary, and you may want to know that!</title>
<link rel="stylesheet" href="/static/style.css">
</head>
<body>
<div class="page">
<div class="meta">
<header>
<h2><a href="/">naming is hard</a></h2>
</header>
</div>
<div class="article">
<section id="Go-can-remove-code-from-the-binary-and-you-may-want-to-know-that">
<a class="heading-anchor" href="#Go-can-remove-code-from-the-binary-and-you-may-want-to-know-that"><h1 date="2025/02/23">Go can remove code from the binary, and you may want to know that!</h1>
</a><p>One year ago I implemented a simple <code class="inline">Go</code> linter which leverages the power of the <code class="inline">Go</code> compiler to highlight potential bugs and it was able to find real issues in the software (<a href="https://github.com/spacemeshos/go-spacemesh/pull/5873">go-spacemesh</a>, <a href="https://github.com/cloudflare/cloudflared/pull/1139">cloudflared</a>, <a href="https://github.com/wal-g/wal-g/pull/1612">wal-g</a>, <a href="https://github.com/francoismichel/ssh3/pull/82">ssh3</a>, <a href="https://github.com/apache/answer/pull/1229">answer</a>, <a href="https://github.com/pdfcpu/pdfcpu/pull/762">pdfcpu</a>).</p>
<p>The linter uses a quite interesting approach as it directly uses <code class="inline">Go</code> compiler output to find places of code which were removed by the compiler – allowing it to detect non-trivial issues as long as the <code class="inline">Go</code> compiler can performs clever optimizations.</p>
<p>So, let me tell a little story about linter development and meanwhile you can run it (<a href="https://github.com/sivukhin/govanish">govanish</a>) on your project and check if your code vanished from the binary without your approval!</p>
<div class="note">
<p>The idea of using <code class="inline">Go</code> compiler output for a linter is not unique. I have seen another linter with similar approach that validates all functions marked with special comment will be inlined by the compiler – but I can’t find a proper reference to it right now.</p>
</div>
</section>
<section id="Beginning">
<a class="heading-anchor" href="#Beginning"><h2>Beginning</h2>
</a><p>Everything started with me playing around with <code class="inline">Go</code> and suddenly discovering tricky nuance of how <code class="inline">Go</code> interfaces are working.</p>
<p>Internally, every interface instance has two components: one that represents the underlying concrete <strong><strong>type</strong></strong> of the instance and another that represents the actual <strong><strong>value</strong></strong> of the instance. While this is not a surprise, it turns out that for interfaces to be considered equal, the <code class="inline">Go</code> compiler checks <strong><strong>both</strong></strong> components for equality. This behavior starting to play weirdly when you operate with <code class="inline">nil</code> values and <code class="inline">nil</code> interfaces (more detailed explanation can be found <a href="https://trstringer.com/go-nil-interface-and-interface-with-nil-concrete-value/">here</a>).</p>
<p>Look at the following code snippet:</p>
<pre class="language-go"><code><span class="keyword">package</span> <span class="identifier">main</span></code>
<code></code>
<code><span class="keyword">import</span> <span class="string">"net/url"</span></code>
<code></code>
<code><span class="keyword">func</span> <span class="function">alwaysNilType</span>() *<span class="identifier">url</span>.<span class="identifier">Error</span> { <span class="keyword">return</span> <span class="identifier">nil</span> }</code>
<code><span class="keyword">func</span> <span class="function">alwaysNilInterface</span>() <span class="identifier">error</span> { <span class="keyword">return</span> <span class="function">alwaysNilType</span>() }</code>
<code></code>
<code><span class="keyword">func</span> <span class="function">main</span>() {</code>
<code> <span class="keyword">if</span> <span class="identifier">err</span> := <span class="function">alwaysNilInterface</span>(); <span class="identifier">err</span> == <span class="identifier">nil</span> {</code>
<code> <span class="function">panic</span>(<span class="string">"never called"</span>)</code>
<code> }</code>
<code>}</code>
</pre>
<p>Turns out that <code class="inline">alwaysNilInterface</code> always returns interface instance which has <code class="inline">nil</code> value but <strong><strong>non-<code class="inline">nil</code></strong></strong> type information (because type is <code class="inline">*url.Error</code>) and this makes the value from this function unequal to a <code class="inline">nil</code> interface which has both type and value components set to <code class="inline">nil</code>!</p>
<div class="note">
<p>That’s why you should always use <code class="inline">error</code> interface in Go instead of more specific types despite that in other languages you usually <a href="https://enterprisecraftsmanship.com/posts/return-the-most-specific-type/"><em>return the most specific type, accept the most generic type</em></a>)</p>
</div>
</section>
<section id="Assembly">
<a class="heading-anchor" href="#Assembly"><h2>Assembly</h2>
</a><p>After discovering this nuance, I wondered how interface comparison works, which led me to examine the generated assembly for the snippet above (<em>by the way, Golang uses <a href="https://go.dev/doc/asm">Plan9</a> assembler syntax which is annoying sometimes as it’s very hard to find description of instructions</em>). Luckily, Go provides an easy way to inspect the generated assembler code: we can just pass <code class="inline">-gcflags=-S</code> argument to the <code class="inline">go build</code> command. The output of the compiler looks like following:</p>
<pre class="language-asm"><code><span class="comment"># command-line-arguments</span></code>
<code>...</code>
<code>main.main <span class="keyword">STEXT</span> nosplit size=<span class="number">1</span> args=<span class="number">0</span>x0 locals=<span class="number">0</span>x0 funcid=<span class="number">0</span>x0 align=<span class="number">0</span>x0</code>
<code> 0x0000 00000 (/home/sivukhin/code/go-nil-interface/main.go:8) <span class="keyword">TEXT</span> main.main(SB), NOSPLIT|NOFRAME|ABIInternal, <span class="number">$0</span><span class="number">-0</span></code>
<code> 0x0000 00000 (/home/sivukhin/code/go-nil-interface/main.go:8) <span class="keyword">FUNCDATA</span> <span class="number">$0</span>, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)</code>
<code> 0x0000 00000 (/home/sivukhin/code/go-nil-interface/main.go:8) <span class="keyword">FUNCDATA</span> <span class="number">$1</span>, gclocals·g2BeySu+wFnoycgXfElmcg==(SB)</code>
<code> 0x0000 00000 (/home/sivukhin/code/go-nil-interface/main.go:12) <span class="keyword">RET</span></code>
<code> 0x0000 <span class="keyword">c3</span> .</code>
<code> <span class="keyword">rel</span> <span class="number">0</span><span class="number">+0</span> t=R_USEIFACE type:*net/url.Error<span class="number">+0</span></code>
<code> <span class="keyword">rel</span> <span class="number">0</span><span class="number">+0</span> t=R_USEIFACE type:string<span class="number">+0</span></code>
<code>...</code>
</pre>
<p>That’s interesting, because compiler completely removed conditional code from the <code class="inline">main</code> function body and just generates “no-op” executable. This is kind of expected behaviour: optimizing compiler should simplify code and remove “useless” operations – that’s why we love them. But from the developer perspective it’s actually bit suspicious, because the removal of code from the binary may indicate that something unexpected has happened: we wrote code <code class="inline">err == nil</code> condition on purpose and it’s shouldn’t result in constant result independent of the function inputs.</p>
<p>There is another interesting bit in the generated assembly – you can notice that every instruction annotated with the line of code which “produced” the sequence of assembler operations. That’s cool! Let’s look at which lines are actually in use in the final assembler code based on the compiler output:</p>
<pre class="language-shell"><code><span class="command">$> go test -c -gcflags=-S main.go 2>&1 | grep -Po "main.go:\d+" | uniq</span></code>
<code>main.go:5</code>
<code>main.go:6</code>
<code>main.go:8</code>
<code>main.go:12</code>
</pre>
<p>We can see that line 10 is missing in the output which is the hint that compiler completely removed logic from this line in the final assembly.</p>
<p>That’s actually nice! So, what if we try to utilize this combination of compiler output introspection and the fact that sometimes compiler can delete parts of the code which we definitely wanted to see in the binary in some form? Can we create a linter out from that idea which will allow us to find non-trivial bugs in the go code? How noisy this linter will be? Let’s find out!</p>
</section>
<section id="Govanish">
<a class="heading-anchor" href="#Govanish"><h2>Govanish</h2>
</a><p>So, that’s how <a href="https://github.com/sivukhin/govanish">govanish</a> linter actually works!</p>
<p>It builds your project with the <code class="inline">gcflags=-S</code> option and analyzes which lines of code are missed from the generated assembly. Not surprisingly, on a big projects linter is pretty noisy, that’s why few heuristics were implemented to ignore from analysis few common cases when <code class="inline">Go</code> compiler remove or replace chunk of code logic in the final assembler listing:</p>
<ul>
<li>
Code like <code class="inline">for k := range m { delete(m, k) }</code> is not analyzed because <code class="inline">Go</code> compiler is clever enough to use <code class="inline">runtime.mapclear()</code> function in this case
</li>
<li>
Constant <code class="inline">true</code>/<code class="inline">false</code> conditions are not analyzed, because they are usually written for local debugging and intent is clear that the code should be removed by compiler
</li>
<li>
Type constructors are not analyzed because in most cases they are no-op (e.g. <code class="inline">[]byte(net.IP{})</code>)
</li>
<li>
Platform dependent code is not analyzed, because it will always trigger <code class="inline">govanish</code> on a wrong platform
</li>
<li>
Conditions which uses functions with constant returns (e.g. <code class="inline">return nil</code>) are not analyzed if function is defined in the module
</li>
</ul>
<p>Still, even with these heuristics in place, <code class="inline">govanish</code> produces a lot of false positives especially around error checking code, because a lot of Go functions follow generic approach to return an error while actually they are never failing (e.g. <code class="inline">func (b *strings.Builder) Write(p []byte) (int, error)</code>). But on the other hand I find it fun to run <code class="inline">govanish</code> from time to time on popular projects and discover some bugs with it.</p>
</section>
<section id="Govanish-in-action">
<a class="heading-anchor" href="#Govanish-in-action"><h2>Govanish in action</h2>
</a><p>Let’s see <code class="inline">govanish</code> in action on some real project – I decided to pick the <a href="https://github.com/rook/rook/tree/master">rook</a> repository (there is an obviously an element of cherry-picking as I ran <code class="inline">govanish</code> on a several projects before <code class="inline">rook</code> and detected no real issues there).</p>
<p>So, you can clone the repository, install <code class="inline">govanish</code> and run it:</p>
<pre class="language-shell"><code><span class="command">$> go install github.com/sivukhin/govanish@latest</span></code>
<code><span class="command">$> git clone https://github.com/rook/rook</span></code>
<code><span class="command">$> cd rook</span></code>
<code><span class="command">$> govanish -format github</span></code>
<code>2025/02/23 15:21:34 module path: /home/sivukhin/code/kekek/rook</code>
<code>2025/02/23 15:21:34 ready to compile project at path '/home/sivukhin/code/kekek/rook' for assembly inspection</code>
<code>2025/02/23 15:21:34 ready to parse assembly output</code>
<code>2025/02/23 15:21:40 ready to normalize assembly lines (size 428)</code>
<code>2025/02/23 15:21:41 built func registry: 2446 entries</code>
<code>2025/02/23 15:21:41 ready to analyze module AST</code>
<code>::warning file=pkg/operator/k8sutil/pod.go,line=167,endLine=167::seems like code vanished from compiled binary</code>
<code>::warning file=pkg/operator/k8sutil/pod.go,line=185,endLine=185::seems like code vanished from compiled binary</code>
<code>::warning file=pkg/operator/discover/discover.go,line=440,endLine=444::seems like code vanished from compiled binary</code>
<code>::warning file=pkg/operator/test/client.go,line=99,endLine=99::seems like code vanished from compiled binary</code>
</pre>
<p><code class="inline">govanish</code> identified 4 code regions which do not match ignore heuristics and vanished from the final assembler listing. Let’s look more closely all of them.</p>
<ol>
<li>
<p>In <a href="https://github.com/rook/rook/blob/81d9159c2d6a1393b0c157637d031255e28f8c06/pkg/operator/k8sutil/pod.go#L167"><code class="inline">pkg/operator/k8sutil/pod.go</code></a> file at line <code class="inline">167</code> there is a return value which were removed by Go for some reason.</p>
<pre class="language-go" style="counter-set: listing 159"><code><span class="keyword">func</span> <span class="function">GetRunningPod</span>(<span class="identifier">ctx</span> <span class="identifier">context</span>.<span class="identifier">Context</span>, <span class="identifier">clientset</span> <span class="identifier">kubernetes</span>.<span class="identifier">Interface</span>) (*<span class="identifier">v1</span>.<span class="identifier">Pod</span>, <span class="identifier">error</span>) {</code>
<code> <span class="identifier">podName</span> := <span class="identifier">os</span>.<span class="function">Getenv</span>(<span class="identifier">PodNameEnvVar</span>)</code>
<code> <span class="keyword">if</span> <span class="identifier">podName</span> == <span class="string">""</span> {</code>
<code> <span class="keyword">return</span> <span class="identifier">nil</span>, <span class="identifier">fmt</span>.<span class="function">Errorf</span>(<span class="string">"..."</span>)</code>
<code> }</code>
<code> <span class="identifier">podNamespace</span> := <span class="identifier">os</span>.<span class="function">Getenv</span>(<span class="identifier">PodNamespaceEnvVar</span>)</code>
<code> <span class="keyword">if</span> <span class="identifier">podName</span> == <span class="string">""</span> {</code>
<code> <span class="keyword">return</span> <span class="identifier">nil</span>, <span class="identifier">fmt</span>.<span class="function">Errorf</span>(<span class="string">"..."</span>)</code>
<code> }</code>
<code> ...</code>
</pre>
<p>Here, we actually spotted a real <strong><strong>bug</strong></strong>, because the code obviously need to check <code class="inline">podNamespace</code> variable at line <code class="inline">165</code> instead of duplicate check of <code class="inline">podName</code> variable (<em>by the way, <code class="inline">Goland</code> also warns about this issue with <code class="inline">Condition is always false</code> rule</em>)</p>
</li>
<li>
<p>In <a href="https://github.com/rook/rook/blob/81d9159c2d6a1393b0c157637d031255e28f8c06/pkg/operator/k8sutil/pod.go#L185"><code class="inline">pkg/operator/k8sutil/pod.go</code></a> line <code class="inline">185</code> disappeared from the final assembly but actually compiler emitted necessary logic but attributed assembly listing slightly differently which led <code class="inline">govanish</code> to think that this line actually disappeared.</p>
<pre class="language-go" style="counter-set: listing 180"><code><span class="keyword">func</span> <span class="function">GetMatchingContainer</span>(<span class="identifier">containers</span> []<span class="identifier">v1</span>.<span class="identifier">Container</span>, <span class="identifier">name</span> <span class="identifier">string</span>) (<span class="identifier">v1</span>.<span class="identifier">Container</span>, <span class="identifier">error</span>) {</code>
<code> <span class="keyword">var</span> <span class="identifier">result</span> *<span class="identifier">v1</span>.<span class="identifier">Container</span></code>
<code> <span class="keyword">if</span> <span class="function">len</span>(<span class="identifier">containers</span>) == <span class="number">1</span> {</code>
<code> <span class="comment">// if there is only one pod, use its image rather than require a set container name</span></code>
<code> <span class="identifier">result</span> = &<span class="identifier">containers</span>[<span class="number">0</span>]</code>
<code> }</code>
<code> ...</code>
</pre>
</li>
<li>
<p>In <a href="https://github.com/rook/rook/blob/81d9159c2d6a1393b0c157637d031255e28f8c06/pkg/operator/discover/discover.go#L440-L444"><code class="inline">pkg/operator/discover/discover.go</code></a> file range between lines <code class="inline">440-444</code> were removed by Go and this is also a <strong><strong>bug</strong></strong> because this is an <code class="inline">else if</code> condition which comes after <code class="inline">else if len(filter) >= 0</code> which is always true, making the subsequent <code class="inline">else if</code> condition redundant! (<em>here, <code class="inline">Goland</code> is silent and do not see any issue with the code</em>)</p>
<pre class="language-go" style="counter-set: listing 425"><code>} <span class="keyword">else</span> <span class="keyword">if</span> <span class="function">len</span>(<span class="identifier">filter</span>) >= <span class="number">0</span> {</code>
<code> <span class="keyword">for</span> <span class="identifier">i</span> := <span class="keyword">range</span> <span class="identifier">nodeDevices</span> {</code>
<code> <span class="comment">//TODO support filter based on other keys</span></code>
<code> <span class="identifier">matched</span>, <span class="identifier">err</span> := <span class="identifier">regexp</span>.<span class="function">Match</span>(<span class="identifier">filter</span>, []<span class="function">byte</span>(<span class="identifier">nodeDevices</span>[<span class="identifier">i</span>].<span class="identifier">Name</span>))</code>
<code> <span class="keyword">if</span> <span class="identifier">err</span> == <span class="identifier">nil</span> && <span class="identifier">matched</span> {</code>
<code> <span class="identifier">d</span> := <span class="identifier">cephv1</span>.<span class="identifier">Device</span>{</code>
<code> <span class="identifier">Name</span>: <span class="identifier">nodeDevices</span>[<span class="identifier">i</span>].<span class="identifier">Name</span>,</code>
<code> }</code>
<code> <span class="identifier">claimedDevices</span> = <span class="function">append</span>(<span class="identifier">claimedDevices</span>, <span class="identifier">nodeDevices</span>[<span class="identifier">i</span>])</code>
<code> <span class="identifier">results</span> = <span class="function">append</span>(<span class="identifier">results</span>, <span class="identifier">d</span>)</code>
<code> }</code>
<code> }</code>
<code>} <span class="keyword">else</span> <span class="keyword">if</span> <span class="identifier">useAllDevices</span> {</code>
<code> <span class="keyword">for</span> <span class="identifier">i</span> := <span class="keyword">range</span> <span class="identifier">nodeDevices</span> {</code>
<code> ...</code>
</pre>
</li>
<li>
<p>In <a href="https://github.com/rook/rook/blob/81d9159c2d6a1393b0c157637d031255e28f8c06/pkg/operator/test/client.go#L99"><code class="inline">pkg/operator/test/client.go</code></a> file cast check code were removed from the binary because compiler proved that failure condition can never happen. This is not a bug and pretty frequent false positive case for <code class="inline">govanish</code>.</p>
<pre class="language-go" style="counter-set: listing 96"><code><span class="identifier">fd</span>, <span class="identifier">ok</span> := <span class="identifier">d</span>.(*<span class="identifier">fakediscovery</span>.<span class="identifier">FakeDiscovery</span>)</code>
<code><span class="keyword">if</span> !<span class="identifier">ok</span> {</code>
<code> <span class="function">panic</span>(<span class="identifier">fmt</span>.<span class="function">Errorf</span>(<span class="string">"failed to get fake clientset's fake discovery"</span>))</code>
<code>}</code>
</pre>
</li>
</ol>
<p>Overall, in the <code class="inline">rook</code> repository, <code class="inline">govanish</code> found two bugs with a 50% false positive rate – which is pretty cool! While the average false positive rate of the linter is higher, <code class="inline">govanish</code> has still proven to be useful and capable of finding bugs that other linters miss. Give it a try and see how much code <code class="inline">Go</code> removes without your approval!</p>
</section>
</div>
<hr/>
<div class="footer">
<p><a href="https://github.com/sivukhin/sivukhin.github.io">github link</a> (made with <a href="https://github.com/sivukhin/godjot">godjot</a> and <a href="https://github.com/sivukhin/gopeg">gopeg</a>)</p>
</div>
</div>
</body>
</html>