silx.math.fit.peaks: Fixed -Wuse-after-free warning

[t20100]

Checklist:

• The PR title is formatted as: <Module or Topic>: <Action> <Summary> (see contributing guidelines)

---

This PR aims at fixing Wuse-after-free warning with recent gcc (12 or 14?) reported in #4139.
At least, it fixes the case of a first successful realloc for peaks0 and a failed second realloc for relevances0 (in which case peaks0 points to freed memory).

closes #4139

[t20100]

Tested in a debian:sid docker (with gcc-13), this PR solves the compilation warnings.

[woutdenolf]

So from this and this

1) On success, returns the pointer to the beginning of newly allocated memory.
The original pointer ptr is invalidated and any access to it is undefined behavior
(even if reallocation was in-place).

2) On failure, returns a null pointer. The original pointer ptr remains valid.

3) If you pass a null pointer for ptr, realloc behaves just like ‘malloc (newsize)’.
Otherwise, if newsize is zero realloc frees the block and returns NULL.
Otherwise, if realloc cannot reallocate the requested size it returns NULL
and sets errno; the original block is left undisturbed. 

realloc_peaks = realloc(peaks0, max_npeaks * sizeof(double));
if (realloc_peaks == NULL) {
    // 2)
    printf("Error: failed to extend memory for peaks array.");
    *peaks = peaks0;
    *relevances = relevances0;   // ????
    return(-n_peaks);
} else {
     // 1) or 3)  -> do not use peaks0 anymore
    peaks0 = realloc_peaks;
     // where is peaks pointing to? Shouldn't we do *peaks = peaks0; ?
}

realloc_relevances = realloc(relevances0, max_npeaks * sizeof(double));
if (realloc_relevances == NULL) {
     // 2)
    printf("Error: failed to extend memory for peak relevances array.");
    *peaks = peaks0;     // ????
    *relevances = relevances0;
    return(-n_peaks);
}
else {
     // 1) or 3)  -> do not use relevances0 anymore
    relevances0 = realloc_relevances;
     // where is relevances pointing to? Shouldn't we do *relevances = relevances0; ?
}

The original version has a different behavior

realloc_peaks = realloc(peaks0, max_npeaks * sizeof(double));
realloc_relevances = realloc(relevances0, max_npeaks * sizeof(double));
if (realloc_peaks == NULL || realloc_relevances == NULL) {
    printf("Error: failed to extend memory for peaks array.");
    *peaks = peaks0;
    *relevances = relevances0;
    return(-n_peaks);
}
else {
    peaks0 = realloc_peaks;
    relevances0 = realloc_relevances;
}

If one of the two realloc fails, you keep the original pointers but e.g. peaks0 is invalid when realloc_peaks!=NULL. So the compiler warning is warranted.

However in the refactored form, what is the effect if one realloc fails and the other not?

[woutdenolf]

Looking at the code I see this in the beginning

/* Output pointers */
*peaks = peaks0;
*relevances = relevances0;

And at the end this

*peaks = peaks0;
*relevances = relevances0;
return (n_peaks);

Which probably why it was done as well in the early returns.

But why? Since you do it in the beginning, shouldn't you only do it again when the peaks0 or relevances0 changes?

I'm thinking it should be

/* Output pointers */
*peaks = peaks0;
*relevances = relevances0;

...
    
    realloc_peaks = realloc(peaks0, max_npeaks * sizeof(double));
    if (realloc_peaks == NULL) {
        printf("Error: failed to extend memory for peaks array.");
        return (-n_peaks);
    } else {
        peaks0 = realloc_peaks;
        *peaks = peaks0;
    }
    
    realloc_relevances = realloc(relevances0, max_npeaks * sizeof(double));
    if (realloc_relevances == NULL) {
        printf("Error: failed to extend memory for peak relevances array.");
        return (-n_peaks);
    }
    else {
        relevances0 = realloc_relevances;
        *relevances = relevances0;
    }

...

return (n_peaks);

[woutdenolf]

Or we do the opposite

...
    
    realloc_peaks = realloc(peaks0, max_npeaks * sizeof(double));
    if (realloc_peaks == NULL) {
        printf("Error: failed to extend memory for peaks array.");
        *peaks = peaks0;
        *relevances = relevances0;
        return (-n_peaks);
    } else {
        peaks0 = realloc_peaks;
    }
    
    realloc_relevances = realloc(relevances0, max_npeaks * sizeof(double));
    if (realloc_relevances == NULL) {
        printf("Error: failed to extend memory for peak relevances array.");
        *peaks = peaks0;
        *relevances = relevances0;
        return (-n_peaks);
    }
    else {
        relevances0 = realloc_relevances;
    }

...

*peaks = peaks0;
*relevances = relevances0;
return (n_peaks);

But a mixture of the two is confusing imo.

[t20100]

If one of the two realloc fails, you keep the original pointers but e.g. peaks0 is invalid when realloc_peaks!=NULL. So the compiler warning is warranted.
However in the refactored form, what is the effect if one realloc fails and the other not?

The refactored form returns 2 valid pointers which is the expected behavior IMO.
One of the 2 buffers will have more memory than the other, but the returned value tells how main items are in the buffer, so you can retrieve the content no matter the size of the allocated memory.

Since you do it in the beginning, shouldn't you only do it again when the peaks0 or relevances0 changes?

Indeed, *peaks and *relevances are updated more than needed, I'll update that.

[t20100]

I updated to this PR to update peaks and relevances output pointers when reallocated only.

[woutdenolf]

Much clearer. Thanks!