we maintain a vendored copy of rrlpd inside a GSM test network and ran the RRLP assistance path against real handsets. With skeletons generated by asn1c 0.9.28, the RRLP assistance delivery is broken in several ways, all of them API misuse from the days when these fields were generated as INTEGER_t octet strings instead of the plain native types asn1c emits today: 1. MoreAssDataToBeSent_t is a NativeEnumerated (typedef long). _rrlp_create_gps_assist_pdu() calls asn_long2INTEGER() on a calloc(1, sizeof(long)) allocation: asn_long2INTEGER writes an INTEGER_t {buf, size} (16 bytes) past the 8-byte chunk and leaves the enum holding garbage (we observed 829434072 where 0..1 is valid). uper_encode_to_new_buffer() then rejects every AssistanceData PDU, so rrlp_gps_assist_pdus() always fails and the MS never receives any assistance data. 2. The same type confusion crashes rrlpd outright on incoming MsrPosition-Rsp messages: asn_INTEGER2long() on locationError->locErrorReason (NativeInteger, typedef long) reads the error value itself as an {buf, size} struct pointer. A handset answering a position request with locationError gpsAssDataMissing (= 6) segfaults the daemon dereferencing address 0x6. The protocolError/errorCause path has the same bug. 3. _rrlp_add_almanac() and _rrlp_add_ephemeris() attach their Almanac/NavigationModel container even when there is nothing to put in it (no almanac content, or no SV passing the MS's satellite mask). An attached but empty almanacList/navModelList violates its SIZE(1..32) / SIZE(1..16) constraint and the PER encoder rejects the whole message. They now attach only when at least one element is added; the callers keep a PDU with no content open for the next element, and still send it as an (empty but valid) assistance answer so the MS is never left waiting. 4. If no PDU survives (all requested elements skipped), the "set last message flag" code indexed lst_pdu[-1]. That case now returns -ENOENT instead. 5. 1ULL << (sv_id - 1) is undefined for sv_id 0 or > 64; the mask test in _rrlp_add_ephemeris() now bounds sv_id first. Fix 1 is the interesting one for anyone vendoring this code: it means rrlp_gps_assist_pdus() has never produced a single valid assistance PDU under modern asn1c, and RRLP ms-based sessions stall after the MS answers locationError gpsAssDataMissing asking for assistance. Verified on the host against the 0.9.28 skeletons and on a live GSM network: an MS answering with locationError 6 + additionalAssistanceData now gets its ephemeris/almanac/UTC/ionosphere assistance in several AssistanceData messages and proceeds to an actual position attempt. rrlpd/src/rrlp.c | 69 ++++++++++++++++++--------- rrlpd/src/main.c | 28 ++++++++-------- 2 files changed, 95 insertions(+), 49 deletions(-) Signed-off-by: Daumantas <daumantas.balakauskas@gmail.com> Also available as a branch, in case pulling is easier than applying the pasted patch: https://github.com/translibrius/osmocom-lcs/tree/rrlpd-fix-asn1c-api-misuse --- --- a/rrlpd/src/rrlp.c +++ b/rrlpd/src/rrlp.c @@ -417,14 +417,10 @@ free(pdu); return NULL; } - /* last message or more messages ? */ - if(iLast) { - if(asn_long2INTEGER(moreAssDataToBeSent, MoreAssDataToBeSent_noMoreMessages) != 0) - fprintf(stderr, "encode error\n"); - } else { - if(asn_long2INTEGER(moreAssDataToBeSent, MoreAssDataToBeSent_moreMessagesOnTheWay) != 0) - fprintf(stderr, "encode error\n"); - } + /* MoreAssDataToBeSent is a NativeEnumerated (plain long); asn_long2INTEGER + * would treat it as an INTEGER_t and write past the 8-byte allocation. */ + *moreAssDataToBeSent = iLast ? MoreAssDataToBeSent_noMoreMessages + : MoreAssDataToBeSent_moreMessagesOnTheWay; if (o_gps_ad) *o_gps_ad = gps_ad; @@ -642,6 +638,12 @@ if (!(gps_ad->fields & GPS_FIELD_ALMANAC)) return -EINVAL; + /* Nothing (left) to add: leave the Almanac absent. An attached but + * empty almanacList violates its SIZE(1..32) constraint and the PER + * encoder rejects the whole message. */ + if (*start >= gps_alm->n_sv) + return 0; + rrlp_alm = calloc(1, sizeof(*rrlp_alm)); if (!rrlp_alm) return -ENOMEM; @@ -676,6 +678,21 @@ if (!(gps_ad->fields & GPS_FIELD_EPHEMERIS)) return -EINVAL; + /* Find the first SV that passes the MS's satellite mask. Without a + * match, leave the NavigationModel absent: an attached but empty + * navModelList violates its SIZE(1..16) constraint and the PER + * encoder rejects the whole message. Also keeps sv_id outside + * 1..64 away from the shift below. */ + for (i = *start; i < gps_eph->n_sv; i++) { + if (gps_eph->svs[i].sv_id >= 1 && gps_eph->svs[i].sv_id <= 64 && + (mask & (1ULL << (gps_eph->svs[i].sv_id - 1)))) + break; + } + if (i >= gps_eph->n_sv) { + *start = i; + return 0; + } + rrlp_nav = calloc(1, sizeof(*rrlp_nav)); if (!rrlp_nav) return -ENOMEM; @@ -683,8 +700,9 @@ if (count == -1) count = gps_eph->n_sv - *start; - for (i=*start,j=0; (j<count) && (i<gps_eph->n_sv); i++) { - if (!(mask & (1ULL<<(gps_eph->svs[i].sv_id-1)))) /* Dieter: CHECK */ + for (j=0; (j<count) && (i<gps_eph->n_sv); i++) { + if (gps_eph->svs[i].sv_id < 1 || gps_eph->svs[i].sv_id > 64 || + !(mask & (1ULL<<(gps_eph->svs[i].sv_id-1)))) /* Dieter: CHECK */ continue; struct NavModelElement *nme; nme = calloc(1, sizeof(*nme)); @@ -795,8 +813,12 @@ if (rv < 0) goto error; - lst_pdu[lst_cnt++] = rrlp_pdu; - rrlp_pdu = NULL; + /* keep the PDU open when no element was added; the + * next element may still fill it */ + if (rrlp_gps_ad->controlHeader.almanac) { + lst_pdu[lst_cnt++] = rrlp_pdu; + rrlp_pdu = NULL; + } } while (rv); } #endif @@ -824,16 +846,35 @@ #elif 0 /* all sets in one PDU, too large */ rv = _rrlp_add_ephemeris(rrlp_gps_ad, gps_ad, &i, -1, req->eph_svs); #endif - lst_pdu[lst_cnt++] = rrlp_pdu; - rrlp_pdu = NULL; + if (rv < 0) + goto error; + /* keep the PDU open when no element was added */ + if (rrlp_gps_ad->controlHeader.navigationModel) { + lst_pdu[lst_cnt++] = rrlp_pdu; + rrlp_pdu = NULL; + } } while (rv); } #endif - /* set last message flag */ - if(asn_long2INTEGER(lst_pdu[lst_cnt - 1]->component.choice.assistanceData.moreAssDataToBeSent, MoreAssDataToBeSent_noMoreMessages) != 0) - fprintf(stderr, "encode error\n"); + /* A skipped element may have left its PDU open with no content; send + * it anyway so the MS still gets an assistance answer. */ + if (rrlp_pdu) { + lst_pdu[lst_cnt++] = rrlp_pdu; + rrlp_pdu = NULL; + } + + /* All requested elements were skipped or had no content: nothing to + * send. Fail rather than indexing lst_pdu[-1] below. */ + if (lst_cnt == 0) { + rv = -ENOENT; + goto error; + } + + /* set last message flag (NativeEnumerated: plain long, see above) */ + *lst_pdu[lst_cnt - 1]->component.choice.assistanceData.moreAssDataToBeSent = + MoreAssDataToBeSent_noMoreMessages; /* Serialize & Release all PDUs */ for (i=0; i<lst_cnt && i<o_max_pdus; i++) { --- a/rrlpd/src/main.c +++ b/rrlpd/src/main.c @@ -350,21 +350,19 @@ if(msrPositionRsp->locationError) { printd("---- locationError\n"); - if(asn_INTEGER2long(&msrPositionRsp->locationError->locErrorReason, &l) == 0) { - asn_INTEGER_specifics_t *specs = (asn_INTEGER_specifics_t *)asn_DEF_LocErrorReason.specifics; - const asn_INTEGER_enum_map_t *el; - el = INTEGER_map_value2enum(specs, l); - if(el) { - printd("---- locErrorReason = %ld (%s)\n", l, el->enum_name); - sprintf(reply, "locationError %ld (%s)", l, el->enum_name); - } else { - printd("---- locErrorReason = %ld\n", l); - sprintf(reply, "locationError %ld", l); - } + /* LocErrorReason is a NativeInteger (plain long), not an + * INTEGER_t; asn_INTEGER2long would read the value itself + * as a {buf,size} struct pointer and crash. */ + l = msrPositionRsp->locationError->locErrorReason; + asn_INTEGER_specifics_t *specs = (asn_INTEGER_specifics_t *)asn_DEF_LocErrorReason.specifics; + const asn_INTEGER_enum_map_t *el; + el = INTEGER_map_value2enum(specs, l); + if(el) { + printd("---- locErrorReason = %ld (%s)\n", l, el->enum_name); + sprintf(reply, "locationError %ld (%s)", l, el->enum_name); } else { - printd("---- locErrorReason: decode error\n"); - strcpy(reply, "locationError ??"); - l = 9999; + printd("---- locErrorReason = %ld\n", l); + sprintf(reply, "locationError %ld", l); } if(msrPositionRsp->locationError->additionalAssistanceData) { printd("------ additionalAssistanceData\n"); @@ -582,20 +580,17 @@ case RRLP_Component_PR_protocolError: printd("-- protocolError\n"); ProtocolError_t *protocolError = &pdu->component.choice.protocolError; - if(asn_INTEGER2long(&protocolError->errorCause, &l) == 0) { - asn_INTEGER_specifics_t *specs = (asn_INTEGER_specifics_t *)asn_DEF_ErrorCodes.specifics; - const asn_INTEGER_enum_map_t *el; - el = INTEGER_map_value2enum(specs, l); - if(el) { - printd("---- errorCause = %ld (%s)\n", l, el->enum_name); - sprintf(reply, "protocolError %ld (%s)", l, el->enum_name); - } else { - printd("---- errorCause = %ld\n", l); - sprintf(reply, "protocolError %ld", l); - } + /* ErrorCodes is a NativeInteger (plain long), like LocErrorReason */ + l = protocolError->errorCause; + asn_INTEGER_specifics_t *specs = (asn_INTEGER_specifics_t *)asn_DEF_ErrorCodes.specifics; + const asn_INTEGER_enum_map_t *el; + el = INTEGER_map_value2enum(specs, l); + if(el) { + printd("---- errorCause = %ld (%s)\n", l, el->enum_name); + sprintf(reply, "protocolError %ld (%s)", l, el->enum_name); } else { - printd("---- errorCause: decode error\n"); - strcpy(reply, "protocolError ??"); + printd("---- errorCause = %ld\n", l); + sprintf(reply, "protocolError %ld", l); } rrlp_sub_state->state = RRLP_ERROR;