Harald Welte has submitted this change and it was merged. Change subject: GSUP: check osmo_gsup_encode() result ......................................................................
GSUP: check osmo_gsup_encode() result Check and handle gracefully any error which might appear in osmo_gsup_encode() - mark corresponding functions with warn_unused_result attribute to make sure this failure is always checked against. Change-Id: I4551212011fb0bd898c020a183756ed7a9afb9e5 Related: OS#2864 --- M include/osmocom/msc/vlr.h M src/libcommon/gsup_test_client.c M src/libvlr/vlr.c M src/libvlr/vlr_auth_fsm.c M src/libvlr/vlr_core.h M src/libvlr/vlr_lu_fsm.c 6 files changed, 69 insertions(+), 29 deletions(-) Approvals: Harald Welte: Looks good to me, approved Jenkins Builder: Verified diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h index 054a18e..c4b8cf6 100644 --- a/include/osmocom/msc/vlr.h +++ b/include/osmocom/msc/vlr.h @@ -283,7 +283,7 @@ int vlr_subscr_rx_auth_resp(struct vlr_subscr *vsub, bool is_r99, bool is_utran, const uint8_t *res, uint8_t res_len); int vlr_subscr_rx_auth_fail(struct vlr_subscr *vsub, const uint8_t *auts); -int vlr_subscr_tx_auth_fail_rep(const struct vlr_subscr *vsub); +int vlr_subscr_tx_auth_fail_rep(const struct vlr_subscr *vsub) __attribute__((warn_unused_result)); void vlr_subscr_rx_ciph_res(struct vlr_subscr *vsub, struct vlr_ciph_result *res); int vlr_subscr_rx_tmsi_reall_compl(struct vlr_subscr *vsub); int vlr_subscr_rx_imsi_detach(struct vlr_subscr *vsub); @@ -374,7 +374,7 @@ uint32_t vlr_timer(struct vlr_instance *vlr, uint32_t timer); int vlr_subscr_changed(struct vlr_subscr *vsub); -int vlr_subscr_purge(struct vlr_subscr *vsub); +int vlr_subscr_purge(struct vlr_subscr *vsub) __attribute__((warn_unused_result)); void vlr_subscr_cancel(struct vlr_subscr *vsub, enum gsm48_gmm_cause cause); diff --git a/src/libcommon/gsup_test_client.c b/src/libcommon/gsup_test_client.c index be8e768..963b495 100644 --- a/src/libcommon/gsup_test_client.c +++ b/src/libcommon/gsup_test_client.c @@ -101,44 +101,59 @@ } /* allocate + generate + send Send-Auth-Info */ -int req_auth_info(const char *imsi) +static int req_auth_info(const char *imsi) { struct imsi_op *io = imsi_op_alloc(g_gc, imsi, IMSI_OP_SAI); struct osmo_gsup_message gsup = {0}; struct msgb *msg = msgb_alloc_headroom(1200, 200, __func__); + int rc; OSMO_STRLCPY_ARRAY(gsup.imsi, io->imsi); gsup.message_type = OSMO_GSUP_MSGT_SEND_AUTH_INFO_REQUEST; - osmo_gsup_encode(msg, &gsup); + rc = osmo_gsup_encode(msg, &gsup); + if (rc < 0) { + printf("%s: encoding failure (%s)\n", imsi, strerror(-rc)); + return rc; + } return gsup_client_send(g_gc, msg); } /* allocate + generate + send Send-Auth-Info */ -int req_loc_upd(const char *imsi) +static int req_loc_upd(const char *imsi) { struct imsi_op *io = imsi_op_alloc(g_gc, imsi, IMSI_OP_LU); struct osmo_gsup_message gsup = {0}; struct msgb *msg = msgb_alloc_headroom(1200, 200, __func__); + int rc; OSMO_STRLCPY_ARRAY(gsup.imsi, io->imsi); gsup.message_type = OSMO_GSUP_MSGT_UPDATE_LOCATION_REQUEST; - osmo_gsup_encode(msg, &gsup); + rc = osmo_gsup_encode(msg, &gsup); + if (rc < 0) { + printf("%s: encoding failure (%s)\n", imsi, strerror(-rc)); + return rc; + } return gsup_client_send(g_gc, msg); } -int resp_isd(struct imsi_op *io) +static int resp_isd(struct imsi_op *io) { struct osmo_gsup_message gsup = {0}; struct msgb *msg = msgb_alloc_headroom(1200, 200, __func__); + int rc; OSMO_STRLCPY_ARRAY(gsup.imsi, io->imsi); gsup.message_type = OSMO_GSUP_MSGT_INSERT_DATA_RESULT; - osmo_gsup_encode(msg, &gsup); + rc = osmo_gsup_encode(msg, &gsup); + if (rc < 0) { + printf("%s: encoding failure (%s)\n", io->imsi, strerror(-rc)); + return rc; + } imsi_op_release(io); @@ -148,7 +163,7 @@ /* receive an incoming GSUP message */ static void imsi_op_rx_gsup(struct imsi_op *io, const struct osmo_gsup_message *gsup) { - int is_error = 0; + int is_error = 0, rc; if (OSMO_GSUP_IS_MSGT_ERROR(gsup->message_type)) { imsi_op_stats[io->type].num_rx_error++; @@ -160,7 +175,9 @@ case IMSI_OP_SAI: printf("%s; SAI Response%s\n", io->imsi, is_error ? ": ERROR" : ""); /* now that we have auth tuples, request LU */ - req_loc_upd(io->imsi); + rc = req_loc_upd(io->imsi); + if (rc < 0) + printf("Failed to request Location Update for %s\n", io->imsi); imsi_op_release(io); break; case IMSI_OP_LU: @@ -169,7 +186,9 @@ break; case IMSI_OP_ISD: printf("%s; ISD Request%s\n", io->imsi, is_error ? ": ERROR" : ""); - resp_isd(io); + rc = resp_isd(io); + if (rc < 0) + printf("Failed to insert subscriber data for %s\n", io->imsi); break; default: printf("%s: Unknown\n", io->imsi); @@ -283,9 +302,14 @@ for (i = 0; i < 10000; i++) { unsigned long long imsi = 901790000000000 + i; - char imsi_buf[17]; + char imsi_buf[17] = { 0 }; + int rc; + snprintf(imsi_buf, sizeof(imsi_buf), "%015llu", imsi); - req_auth_info(imsi_buf); + rc = req_auth_info(imsi_buf); + if (rc < 0) + printf("Failed to request Auth Info for %s\n", imsi_buf); + osmo_select_main(0); } diff --git a/src/libvlr/vlr.c b/src/libvlr/vlr.c index 96627ed..f73eeb4 100644 --- a/src/libvlr/vlr.c +++ b/src/libvlr/vlr.c @@ -130,7 +130,11 @@ { struct msgb *msg = gsup_client_msgb_alloc(); - osmo_gsup_encode(msg, gsup_msg); + int rc = osmo_gsup_encode(msg, gsup_msg); + if (rc < 0) { + LOGP(DVLR, LOGL_ERROR, "GSUP encoding failure: %s\n", strerror(-rc)); + return rc; + } if (!vlr->gsup_client) { LOGP(DVLR, LOGL_NOTICE, "GSUP link is down, cannot " @@ -498,8 +502,10 @@ struct osmo_gsup_message *gsup_msg) { if (OSMO_GSUP_IS_MSGT_REQUEST(gsup_msg->message_type)) { - vlr_tx_gsup_error_reply(vlr, gsup_msg, - GMM_CAUSE_IMSI_UNKNOWN); + int rc = vlr_tx_gsup_error_reply(vlr, gsup_msg, GMM_CAUSE_IMSI_UNKNOWN); + if (rc < 0) + LOGP(DVLR, LOGL_ERROR, "Failed to send error reply for IMSI %s\n", gsup_msg->imsi); + LOGP(DVLR, LOGL_NOTICE, "Unknown IMSI %s, discarding GSUP request " "of type 0x%02x\n", @@ -775,7 +781,7 @@ struct osmo_gsup_message *gsup_msg) { struct osmo_gsup_message gsup_reply = {0}; - int is_update_procedure = !gsup_msg->cancel_type || + int rc, is_update_procedure = !gsup_msg->cancel_type || gsup_msg->cancel_type == OSMO_GSUP_CANCEL_TYPE_UPDATE; LOGVSUBP(LOGL_INFO, vsub, "Cancelling MS subscriber (%s)\n", @@ -783,11 +789,11 @@ "update procedure" : "subscription withdraw"); gsup_reply.message_type = OSMO_GSUP_MSGT_LOCATION_CANCEL_RESULT; - vlr_subscr_tx_gsup_message(vsub, &gsup_reply); + rc = vlr_subscr_tx_gsup_message(vsub, &gsup_reply); vlr_subscr_cancel(vsub, gsup_msg->cause); - return 0; + return rc; } /* Incoming handler for GSUP from HLR. @@ -812,9 +818,11 @@ if (!gsup.imsi[0]) { LOGP(DVLR, LOGL_ERROR, "Missing IMSI in GSUP message\n"); - if (OSMO_GSUP_IS_MSGT_REQUEST(gsup.message_type)) - vlr_tx_gsup_error_reply(vlr, &gsup, - GMM_CAUSE_INV_MAND_INFO); + if (OSMO_GSUP_IS_MSGT_REQUEST(gsup.message_type)) { + rc = vlr_tx_gsup_error_reply(vlr, &gsup, GMM_CAUSE_INV_MAND_INFO); + if (rc < 0) + LOGP(DVLR, LOGL_ERROR, "Failed to send error reply for IMSI %s\n", gsup.imsi); + } rc = -GMM_CAUSE_INV_MAND_INFO; goto msgb_free_and_return; } diff --git a/src/libvlr/vlr_auth_fsm.c b/src/libvlr/vlr_auth_fsm.c index d14ae8e..51e22c9 100644 --- a/src/libvlr/vlr_auth_fsm.c +++ b/src/libvlr/vlr_auth_fsm.c @@ -207,8 +207,11 @@ /* If authentication hasn't even started, e.g. the HLR sent no auth * info, then we also don't need to tell the HLR about an auth failure. */ - if (afp->auth_requested) - vlr_subscr_tx_auth_fail_rep(vsub); + if (afp->auth_requested) { + int rc = vlr_subscr_tx_auth_fail_rep(vsub); + if (rc < 0) + LOGVSUBP(LOGL_ERROR, vsub, "Failed to communicate AUTH failure to HLR\n"); + } } /* Terminate the Auth FSM Instance and notify parent */ @@ -281,7 +284,9 @@ /* Check if we have vectors available */ if (!vlr_subscr_has_auth_tuple(vsub, afp->auth_tuple_max_reuse_count)) { /* Obtain_Authentication_Sets_VLR */ - vlr_subscr_req_sai(vsub, NULL, NULL); + int rc = vlr_subscr_req_sai(vsub, NULL, NULL); + if (rc < 0) + LOGPFSM(fi, "Failed to request Authentication Sets from VLR\n"); osmo_fsm_inst_state_chg(fi, VLR_SUB_AS_NEEDS_AUTH_WAIT_AI, GSM_29002_TIMER_M, 0); } else { @@ -374,7 +379,7 @@ case VLR_AUTH_E_MS_AUTH_FAIL: if (par->auts) { /* First failure, start re-sync attempt */ - vlr_subscr_req_sai(vsub, par->auts, + rc = vlr_subscr_req_sai(vsub, par->auts, vsub->last_tuple->vec.rand); osmo_fsm_inst_state_chg(fi, VLR_SUB_AS_NEEDS_AUTH_WAIT_SAI_RESYNC, diff --git a/src/libvlr/vlr_core.h b/src/libvlr/vlr_core.h index bf6314d..a6585be 100644 --- a/src/libvlr/vlr_core.h +++ b/src/libvlr/vlr_core.h @@ -5,9 +5,9 @@ struct osmo_gsup_message; const char *vlr_subscr_name(struct vlr_subscr *vsub); -int vlr_subscr_req_lu(struct vlr_subscr *vsub, bool is_ps); +int vlr_subscr_req_lu(struct vlr_subscr *vsub, bool is_ps) __attribute__((warn_unused_result)); int vlr_subscr_req_sai(struct vlr_subscr *vsub, const uint8_t *auts, - const uint8_t *auts_rand); + const uint8_t *auts_rand) __attribute__((warn_unused_result)); struct vlr_subscr *vlr_subscr_alloc(struct vlr_instance *vlr); void vlr_subscr_update_tuples(struct vlr_subscr *vsub, const struct osmo_gsup_message *gsup); diff --git a/src/libvlr/vlr_lu_fsm.c b/src/libvlr/vlr_lu_fsm.c index 6c8b53a..0ac5b9a 100644 --- a/src/libvlr/vlr_lu_fsm.c +++ b/src/libvlr/vlr_lu_fsm.c @@ -72,11 +72,14 @@ void *data) { struct vlr_subscr *vsub = fi->priv; + int rc; OSMO_ASSERT(event == UPD_HLR_VLR_E_START); /* Send UpdateLocation to HLR */ - vlr_subscr_req_lu(vsub, vsub->vlr->cfg.is_ps); + rc = vlr_subscr_req_lu(vsub, vsub->vlr->cfg.is_ps); + if (rc < 0) + LOGPFSML(fi, LOGL_ERROR, "Failed to send UpdateLocation to HLR\n"); osmo_fsm_inst_state_chg(fi, UPD_HLR_VLR_S_WAIT_FOR_DATA, LU_TIMEOUT_LONG, 0); } -- To view, visit https://gerrit.osmocom.org/6010 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: merged Gerrit-Change-Id: I4551212011fb0bd898c020a183756ed7a9afb9e5 Gerrit-PatchSet: 6 Gerrit-Project: osmo-msc Gerrit-Branch: master Gerrit-Owner: Max <msur...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Max <msur...@sysmocom.de>