<p style="white-space: pre-wrap; word-wrap: break-word;">-1 because it should be ntohs.</p><p>Patch set 3:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/13608">View Change</a></p><p>7 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13608/3//COMMIT_MSG">Commit Message:</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/13608/3//COMMIT_MSG@12">Patch Set #3, Line 12:</a> <code style="font-family:monospace,monospace">all, without actually checking any credentials database.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So the MS is expected to send inside PAP the values you configure your APN with? (the User+Password fields in the APN menu of android phones).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c">File ggsn/ggsn.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/13608/3/ggsn/ggsn.c@511">Patch Set #3, Line 511:</a> <code style="font-family:monospace,monospace">"Welcome to OsmoGGSN"</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">btw, I think this will be an easy way to figure out remotely if somebody is using OsmoGGSN or not: S […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">If it was AGPL then it'd be funny answering that in osmo-sgsn with a message "go free you code!" and a copy of the license ;)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@518">Patch Set #3, Line 518:</a> <code style="font-family:monospace,monospace">        unsigned int pap_welcome_len = strlen(pap_welcome);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"ARRAY_SIZE(pap_welcome) -1" would make sense too :)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@519">Patch Set #3, Line 519:</a> <code style="font-family:monospace,monospace">      uint8_t pap_out_size = sizeof(struct pap_element) + 1 + pap_welcome_len;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is that +1 for the NULL chat of pap_welcome? please add comment (and add +1 to the end).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@520">Patch Set #3, Line 520:</a> <code style="font-family:monospace,monospace">       struct pap_element *pap_out = alloca(pap_out_size);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">nice, didn't know about alloca()</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@526">Patch Set #3, Line 526:</a> <code style="font-family:monospace,monospace">        if (htons(pap_in->len) > pco_in->length)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">is htons() fine alignment-access wise?<br>And actually it should be ntohs.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@531">Patch Set #3, Line 531:</a> <code style="font-family:monospace,monospace">              peer_id_len = pap_in->data[0];</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I think you are not covering case sizeof(*pap_in) == ntohs(pap_in->len), that is, pap_in->data is empty. In that case you shoul avoid accessing pap_in->data and returning broken.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/13608">change 13608</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/13608"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-ggsn </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: I81875f30f9f1497199253497f84718510747f731 </div>
<div style="display:none"> Gerrit-Change-Number: 13608 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 11 Apr 2019 19:57:42 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>