Change in osmo-ggsn[master]: ggsn: Add minimalistic PAP support

Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Thu Apr 11 19:57:42 UTC 2019


Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/13608 )

Change subject: ggsn: Add minimalistic PAP support
......................................................................


Patch Set 3: Code-Review-1

(7 comments)

-1 because it should be ntohs.

https://gerrit.osmocom.org/#/c/13608/3//COMMIT_MSG
Commit Message:

https://gerrit.osmocom.org/#/c/13608/3//COMMIT_MSG@12
PS3, Line 12: all, without actually checking any credentials database.
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).


https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c
File ggsn/ggsn.c:

https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@511
PS3, Line 511: "Welcome to OsmoGGSN"
> btw, I think this will be an easy way to figure out remotely if somebody is using OsmoGGSN or not: S […]
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 ;)


https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@518
PS3, Line 518: 	unsigned int pap_welcome_len = strlen(pap_welcome);
"ARRAY_SIZE(pap_welcome) -1" would make sense too :)


https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@519
PS3, Line 519: 	uint8_t pap_out_size = sizeof(struct pap_element) + 1 + pap_welcome_len;
Is that +1 for the NULL chat of pap_welcome? please add comment (and add +1 to the end).


https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@520
PS3, Line 520: 	struct pap_element *pap_out = alloca(pap_out_size);
nice, didn't know about alloca()


https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@526
PS3, Line 526: 	if (htons(pap_in->len) > pco_in->length)
is htons() fine alignment-access wise?
And actually it should be ntohs.


https://gerrit.osmocom.org/#/c/13608/3/ggsn/ggsn.c@531
PS3, Line 531: 		peer_id_len = pap_in->data[0];
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.



-- 
To view, visit https://gerrit.osmocom.org/13608
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-ggsn
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I81875f30f9f1497199253497f84718510747f731
Gerrit-Change-Number: 13608
Gerrit-PatchSet: 3
Gerrit-Owner: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 11 Apr 2019 19:57:42 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190411/e749442e/attachment.html>


More information about the gerrit-log mailing list