Fix #1268 - Segmentation Fault with Constant Lines

[TheWitness]

• This issue occurs with the --add-jsontime for lines that are a constant value. In this case, the variable im->gdes[vidx].step is always 0, which results in a division by zero segmentation fault.

[codecov]

Codecov Report

Attention: Patch coverage is 81.81818% with 4 lines in your changes missing coverage. Please review.

Project coverage is 51.11%. Comparing base (4cb775a) to head (250d939).
Report is 8 commits behind head on master.

Files with missing lines	Patch %	Lines
src/rrd_xport.c	81.81%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1269   +/-   ##
=======================================
  Coverage   51.10%   51.11%           
=======================================
  Files          45       45           
  Lines       18251    18259    +8     
=======================================
+ Hits         9328     9333    +5     
- Misses       8923     8926    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[TheWitness]

@oetiker, I'm still getting random segfault's when enabling the --add-jsontime. So, I expect to have a few more updates to this pull as I track them down.

[TheWitness]

It seems that it's with the use of proc_open() in PHP. If I open the command through a pipe, I get the segfault, if I open it using a shell exec, then there are no problems. Still researching.

[TheWitness]

Okay, for whatever reason, when starting RRDtool in this manner, the variable im->gdes[vidx].step can have bogus values while you traverse the array, not only a zero value, but some very large values. So, something in the structure is causing the step to get mangled. This does not happen when you simply shell_exec() the rrdtool command, only when you do:

./rrdtool -
graphv blah....

So, there are a few ways to fix this:

1. Search like made as to how this pattern of behavior starts when using the standard in pipe
2. Look for the step value, it should be consistent throughout right? So, if you track it as you step through the array, and it switches to something that was not seen before, skip that entry
3. Use a range say > 0 and < some number, say 86400.

What do you think?

[TheWitness]

Two debug outputs with the variation in output. I've added a compare for each loop through the element. Note the bad dates in the output at the end of the proc_open_debug_output.gz, it has somehow got lost in space due to some bad data as the timestamps are not aligning with the step as in the shell_exec_debug.output.gz which appears to be correct.

So, this issue must be deeper in how the stdin is being handled vs. the pure shell exec. The rrdtool binary still crashes when using --add-jsontime without the updated patch in strait RRDtool 1.9.0 or the latest develop.

shell_exec_debug.output.gz
proc_open_debug.output.gz

[TheWitness]

Modified code block from testing. I'll update the pull request with the sanitized version which will fix output generation, but not the issue when using the stdin pipe.

    long unsigned int chosen_step = 0;

    /* fill data structure */
    for (dst_row = 0; (int) dst_row < (int) row_cnt; dst_row++) {
        for (i = 0; i < (int) (*col_cnt); i++) {
            long       vidx = im->gdes[ref_list[i]].vidx;
            time_t     now = *start + dst_row * *step;
            long unsigned int chosen_idx;

            if (chosen_step == 0) {
                if (im->gdes[vidx].step > 0) {
                    chosen_step = im->gdes[vidx].step;
                }
            }

//          if (im->gdes[vidx].step == 0 || im->gdes[vidx].step > chosen_step) {
                printf("==========================================================\n");
                printf("now is %lu\n", (unsigned long) now);
                printf("step is %lu\n", im->gdes[vidx].step);
                printf("start is %lu\n", im->gdes[vidx].start);
                printf("ds_cnt is %li\n", im->gdes[vidx].ds_cnt);
                printf("ds is %li\n", im->gdes[vidx].ds);
                printf("chosen_step is %lu\n", chosen_step);
//          }

            if (im->gdes[vidx].step > 0 && im->gdes[vidx].step == chosen_step) {
                chosen_idx = ceil((double) (now - im->gdes[vidx].start) / im->gdes[vidx].step) * im->gdes[vidx].ds_cnt + im->gdes[vidx].ds;
                printf("chosen_idx is %lu\n", chosen_idx);
                (*dstptr++) = im->gdes[vidx].data[chosen_idx];
//                    ceil((double) (now - im->gdes[vidx].start) / im->gdes[vidx].step) * im->gdes[vidx].ds_cnt + im->gdes[vidx].ds];
            }
        }

[TheWitness]

@oetiker,

If you are okay with my is_numeric() hack, this is working as expected now.

Larry

[c72578]

2x typo: contant -> constant

[TheWitness]

2x typo: contant -> constant

Just like me :)

[TheWitness]

@c72578, I don't see that in the DIFF though. Can you be more specific?

[TheWitness]

Got it. I was looking at the code ;)