<p style="white-space: pre-wrap; word-wrap: break-word;">Feel free to merge, I do not want to block the whole patch set.</p><p>Patch set 20:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #d4ffd4;">Code-Review +1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c">File src/mslookup/mslookup.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@128">Patch Set #15, Line 128:</a> <code style="font-family:monospace,monospace">strncmp(a->msisdn, b->msisdn, sizeof(a->msisdn));</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">note that the n is sizeof(), i.e. the maximum buffer size, and not the strlen(). […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Oh, I overlooked it. Fine by me.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup.c@130">Patch Set #15, Line 130:</a> <code style="font-family:monospace,monospace">return 0;</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">we don't know where the id came from, maybe it is random data incoming from the internet. [...]</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Ok, but shouldn't we sanitize/check the received data before passing to this function? If not an assert(), then at least a warning should be here. Or at least a different return value. Not a merge blocker though.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client.c">File src/mslookup/mslookup_client.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202/15/src/mslookup/mslookup_client.c@59">Patch Set #15, Line 59:</a> <code style="font-family:monospace,monospace">talloc_zero</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I use talloc_zero() by principle, and in my coding style I often fully initialize all members shortl […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Well, there is a bad side of zero-initialization: it's harder to see/find bugs. Neither ASAN nor Valgrind would show you that you're using a non-initialized value. That's e.g. why libosmovty could write to stdin/fd=0 (!), because somebody forgot to initialize it properly.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202">change 16202</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-hlr/+/16202"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-hlr </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I83487ab8aad1611eb02e997dafbcb8344da13df1 </div>
<div style="display:none"> Gerrit-Change-Number: 16202 </div>
<div style="display:none"> Gerrit-PatchSet: 20 </div>
<div style="display:none"> Gerrit-Owner: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 09 Jan 2020 17:58:19 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Comment-In-Reply-To: neels <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>