<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 5:<br>I did, maybe it was not clear enough: "I think sanitizing the imsi should be done by caller of sgsn_acl_* based on where the information come from (from the wire or from known sanitized source)."<br></p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Hmm, I was sure I've addressed it already. Anyway, the "where the information come from" is not applicable because it only comes from a single source which is not trusted so we always sanitize it.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">If you know your data is sane there's no need to re-sanitize it.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">That's not our case.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">You should expect the caller of a data struct to provide sane data instead of internally sanitizing it and storing different data from what was provided.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Sorry, you've lost me with "caller of a data struct" - what do you mean by that?</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">It's responsibility of the caller (vty code for instance) to make sure parse of human input is correctly parsed and sanitized.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">I disagree, and the code I've looked over seems to disagree as well. For example, in osmo_bsc_vty.c:</p><ul><li>osmo_mcc_from_str() sanitize data internally</li><li>gsm_parse_reg() regexp compiled and result checked outside of vty</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">In general, I don't see any non-trivial checks done inside vty which I think is the right thing. What would be the advantage of having this check in separate file instead of static function in the same file? We can also move it to libosmocore but I don't see any benefits from keeping it in vty. Do you?</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">You can do checks inside the data structure if you want (I wouldn't), but I'd avoid changing content of the data being handled in there.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">You mean inside function?</p><p style="white-space: pre-wrap; word-wrap: break-word;">The rest would be addressed in a next revision.</p><p><a href="https://gerrit.osmocom.org/12227">View Change</a></p><ul style="list-style: none; padding: 0;"></ul><p>To view, visit <a href="https://gerrit.osmocom.org/12227">change 12227</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/12227"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-sgsn </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ic3dff108148683b107e9edac430a0475283580e9 </div>
<div style="display:none"> Gerrit-Change-Number: 12227 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Max <msuraev@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Stefan Sperling <stsp@stsp.name> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 13 Dec 2018 10:49:18 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>
<div style="display:none"> Gerrit-HasLabels: No </div>