https://osmocom.org/https://osmocom.org/favicon.ico?16647414092023-01-13T05:34:49ZOpen Source Mobile CommunicationsOsmoHLR - Bug #5854: Memory leak: many 1000s of proxy_subscr_listentryhttps://osmocom.org/issues/5854?journal_id=259102023-01-13T05:34:49Zkeith
<ul><li><strong>Subject</strong> changed from <i>Memory leak many 1000s of proxy_subscr_listentry</i> to <i>Memory leak: many 1000s of proxy_subscr_listentry</i></li></ul> OsmoHLR - Bug #5854: Memory leak: many 1000s of proxy_subscr_listentryhttps://osmocom.org/issues/5854?journal_id=259112023-01-13T05:40:42Zkeith
<ul></ul><p>It's not my forte at all, but, if as in proxy_subscr_create_or_update() we have a:</p>
<pre><code class="c syntaxhl"><span class="n">e</span> <span class="o">=</span> <span class="n">talloc_zero</span><span class="p">(</span><span class="n">proxy</span><span class="p">,</span> <span class="k">struct</span> <span class="n">proxy_subscr_listentry</span><span class="p">);</span>
<span class="n">llist_add</span><span class="p">(</span><span class="o">&</span><span class="n">e</span><span class="o">-></span><span class="n">entry</span><span class="p">,</span> <span class="o">&</span><span class="n">proxy</span><span class="o">-></span><span class="n">subscr_list</span><span class="p">);</span>
</code></pre>
<p>then should there be a corresponding talloc_free() some place?</p>
<p>because it's not in the gc which calls:</p>
<pre><code class="c syntaxhl"><span class="kt">int</span> <span class="nf">_proxy_subscr_del</span><span class="p">(</span><span class="k">struct</span> <span class="n">proxy_subscr_listentry</span> <span class="o">*</span><span class="n">e</span><span class="p">)</span>
<span class="p">{</span>
<span class="n">llist_del</span><span class="p">(</span><span class="o">&</span><span class="n">e</span><span class="o">-></span><span class="n">entry</span><span class="p">);</span>
<span class="k">return</span> <span class="mi">0</span><span class="p">;</span>
<span class="p">}</span>
</code></pre>
<p>as far as I can make out, an llist_del() is just setting some values in memory, there no free'ing of any kind.<br />But I don't really know talloc.</p> OsmoHLR - Bug #5854: Memory leak: many 1000s of proxy_subscr_listentryhttps://osmocom.org/issues/5854?journal_id=259122023-01-13T11:00:12Zlaforge
<ul></ul><p>I'm not familiar with this specific code, but indeed it looks like you are correct, the subscr_del should also<br />free the proxy_subscr_listentry, <strong>unless</strong> we still expect it to have some references at this point and expect<br />something else to clean up later. It would be best to get @neeels or <a class="user active" href="https://osmocom.org/users/301771">osmith</a> input on this.</p>
<p>In general, you can simply try to add a talloc_free(e) after the llist_del() and see what happens. If you<br />don't see any segfaults later on, it is a good indication.</p> OsmoHLR - Bug #5854: Memory leak: many 1000s of proxy_subscr_listentryhttps://osmocom.org/issues/5854?journal_id=259152023-01-13T18:51:11Zkeith
<ul></ul><p>Added the talloc_free, and so far, no segfaults.</p>
<p>However, In my branch, I implemented minimal proxy inspection from the vty, and I seem to be having those entries disappear before would be expected.</p>
<p>I continue to investigate. Just tagging <a class="user active" href="https://osmocom.org/users/91">neels</a> again.</p> OsmoHLR - Bug #5854: Memory leak: many 1000s of proxy_subscr_listentryhttps://osmocom.org/issues/5854?journal_id=259232023-01-15T21:47:06Zneelsnhofmeyr@sysmocom.de
<ul></ul><p>you are completely right, it is a memleak. fix is exactly as you were:</p>
<p><a class="external" href="https://gerrit.osmocom.org/c/osmo-hlr/+/30981">https://gerrit.osmocom.org/c/osmo-hlr/+/30981</a></p> OsmoHLR - Bug #5854: Memory leak: many 1000s of proxy_subscr_listentryhttps://osmocom.org/issues/5854?journal_id=259412023-01-17T09:58:55Zosmith
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>Resolved</i></li><li><strong>% Done</strong> changed from <i>0</i> to <i>100</i></li></ul><p>Thanks! Patch has been merged.</p>