5G-MAG / rt-5gms-application-function

5G Media Streaming - Application Function
https://www.5g-mag.com/streaming
Other
11 stars 6 forks source link

Consumption reports not being stored - expectation 'ptr' failed #133

Closed jordijoangimenez closed 8 months ago

jordijoangimenez commented 9 months ago

This may be a similar issue as reported here https://github.com/5G-MAG/rt-5gms-application-function/issues/115

However I'd like to signal another type of error which is reported (the "prt").

02/11 18:35:31.569: [core] ERROR: ogs_talloc_strdup: Expectation `ptr' failed. (../subprojects/open5gs/lib/core/ogs-strings.c:157)
02/11 18:35:33.228: [msaf] ERROR: Unable to create report directory /var/local/log/open5gs/reports/1658e2b8-c90a-41ee-8f14-6720ad177530/consumption_reports (../src/5gmsaf/data-collection.c:100)
02/11 18:35:33.228: [msaf] ERROR: Failed to store Consumption Report for provisioning session [1658e2b8-c90a-41ee-8f14-6720ad177530] (../src/5gmsaf/msaf-m5-sm.c:797)

Changing the path in the msaf.yaml will and passing this config to the AF will not do anything different. The /var/local/log/open5gs/reports folder contains a report created time ago but no others are created.

davidjwbbc commented 9 months ago

Looks like it's not handling a permissions error very elegantly. This should be improved.

davidjwbbc commented 9 months ago

The "ERROR: ogs_talloc_strdup: Expectation 'ptr' failed." also means that a NULL pointer was passed to the string duplication function when it shouldn't be. The AF has it's own function, msaf_strdup(), which does sensible things when a NULL pointer is passed, that error suggests something tried to use ogs_strdup() instead.

Looking at the code there are several instances where ogs_strdup() has been used instead of msaf_strdup(), these should be changed to use the safer function.

jordijoangimenez commented 9 months ago

David, let me know if you want me to do more testing. I was initially wondering about the permission to write on that folder but executing the AF with sudo did not change the bahaviour. I made it run once though, which resulted in only one report stored.

davidjwbbc commented 8 months ago

On Slack @jordijoangimenez and I tracked this down to when a request was being made for ServiceAccessInformation that included an If-None-Match or If-Modified-Since header that matched the current cached ServiceAccessInformation. In which case a 304 status is returned with an empty body.

In the code the empty body was signalled by a NULL pointer which was handled correctly for the body length, but was being passed to ogs_strdup() unchecked, which in turn passes it to a talloc version of strdup that generates an error log output about the string pointer being NULL.

As part of a code tidy up (Issue #139) all ogs_strdup() calls have been replaced by msaf_strdup() which will not try to copy a NULL pointer, and thus will not generate the error.