otel: fix segfaults when otel not configured

[avahahn]

Fixes #1523 and #1526 by adding null checks to cover unconfigured OpenTelemetry cases.

Both tests and traffic were tested on both x86_64 Linux and arm64 Linux.

[ac000]

otel: fix segfaults when otel not configured

This commit adds null checks for the request->otel object that
were missed in the Traceparent and Tracestate routines. This
fix solves issues 1526 and 1523

Signed-off-by: Ava Hahn <[email protected]>

s/null/NULL/

This could also do with some closes and a fixes tag i.e.

...

Closes: https://github.com/nginx/unit/issues/1523
Closes: https://github.com/nginx/unit/issues/1526
Fixes: 9d3dcb800 ("otel: add build tooling to include otel code")
Signed-off-by: Ava Hahn <[email protected]>

and then remove the The fix solves... line.

Did you audit the other functions that assume r->otel is not NULL? E.g.

• nxt_otel_request_error_path()
• nxt_otel_parse_traceparent()
• nxt_otel_parse_tracestate()

Also taking a closer look at nxt_otel_request_error_path()

void                                                                            
nxt_otel_request_error_path(nxt_task_t *task, nxt_http_request_t *r)            
{                                                                               
    if (r->otel->trace == NULL) {                                               
        return;                                                                 
    }                                                                           
                                                                                
    // response headers have been cleared                                       
    nxt_otel_propagate_header(task, r);                                         
                                                                                
    // collect span immediately                                                 
    if (r->otel) {                                                              
        nxt_otel_state_transition(r->otel, NXT_OTEL_COLLECT_STATE);             
    }                                                                           
    nxt_otel_test_and_call_state(task, r);                                      
} 

The if (r->otel) check (aside from the fact it should do an explicit NULL check), looks superfluous.

We've already checked r->otel->trace == NULL, so assuming we didn't crash, then we know r->otel isn't NULL. But maybe we should be checking that at the top of the function?

For the avoidance of any doubt. If it's superfluous it should be removed in a separate commit, OTOH if we should be checking r->otel == NULL at the top of the function then just do it in this commit...

While we're at it and whatever the fix here is it should be done as a separate commit (and then we can decide if it warrants going into 1.34.1), in nxt_otel_propagate_header() we have

if (r->otel->trace_id != NULL) {
...
} else if (r->otel->trace_id == NULL) {
...
} else {
...
}

->trace_id can only be NULL or !NULL, you see where this is going...

[avahahn]

Did you audit the other functions that assume r->otel is not NULL? E.g.

nxt_otel_request_error_path()
nxt_otel_parse_traceparent()
nxt_otel_parse_tracestate()

...

OTOH if we should be checking r->otel == NULL at the top of the function then just do it in this commit...

I am not sure what commit you are looking at but that is what this commit does. Those three specific functions and your OTOH comment are all directly addressed in the patch that github shows. Did you end up with something wildly different somehow?

[ac000]

No, you're right (in my mind I'd applied the patch locally)

Looks good then...