free5gc / upf

Apache License 2.0
32 stars 51 forks source link

When DL data continues to flow without a Paging response, an error will occur due to double memory release in UPF #15

Closed kishiguro closed 3 years ago

muthuramanecs03g commented 3 years ago

Hi Kishiguro,

Could you please provide the root cause of this issue in detail?

Thanks & Regards, Muthuraman

shugo-h commented 3 years ago

Hi Muthraman,

bufBlk, the third argument of PfcpXactLocalCreate(), is carried to PfcpXactUpdateTx() which is called from PfcpXactLocalCreate(). Near the end of PfcpXactUpdateTx(), bufBlk is freed as following:

    localHeader->length = htons(bufBlk->len + headerLen - 4);

    BufblkBuf(fullPacket, bufBlk);
    BufblkFree(bufBlk);

    xact->seq[xact->step].type = localHeader->type;
    xact->seq[xact->step].bufBlk = fullPacket;

Therefore, bufBlk is already freed before calling BufblkFree() in UpfDispatcher() shown below:

        xact = PfcpXactLocalCreate(session->pfcpNode, &header, bufBlk);
        UTLT_Assert(xact, return, "pfcpXactLocalCreate error");
        status = PfcpXactCommit(xact);
        UTLT_Assert(status == STATUS_OK, return, "xact commit error");

        BufblkFree(bufBlk);

It is verbose freeing.

If bufblkPool is already full when freeing bufBlk verbosely, the following log is output:

Pool is full, it may not belong to this pool
muthuramanecs03g commented 3 years ago

Hi Shugo,

Thanks for the update, and my concern on this issue as follows,

status = UpfN4BuildSessionReportRequestDownlinkDataReport(&bufBlk, header.type, session, pdrId); UTLT_Assert(status == STATUS_OK, return, "Build Session Report Request error");

In the above call, On Success, we allocated a buffer in function PfcpBuildMessage(), and no buffer allocated on the failed case but that function return value always STATUS_OK.

   xact = PfcpXactLocalCreate(session->pfcpNode, &header, bufBlk);
  UTLT_Assert(xact, return, "pfcpXactLocalCreate error");

In the above call, On Success, the allocated buffer (bufBlk) freed but not freed when it failed.

    status = PfcpXactCommit(xact);
    UTLT_Assert(status == STATUS_OK, return, "xact commit error");

    BufblkFree(bufBlk);

In the above statement should be called on a failed case, when function call PfcpXactLocalCreate() failed.

Please, validate my comments and let me know.

Thanks, Muthuraman

shugo-h commented 3 years ago

Hi Muthraman,

Thank you for your comment.

I understand your explanation and I agree with your suggestion. We will fix the patch and push it again.

kishiguro commented 3 years ago

@muthuramanecs03g hi, @shugo-h pushed update to PR. how about now?

muthuramanecs03g commented 3 years ago

Hi Kishiguro/Shugo,

The given PR# looks good, we will merge it.

Thanks. Muthuraman