Collectd: rrdtool is broken in 5.9.1

[faxm0dem]

rrdtool plugin: failed to build values string

[faxm0dem]

other plugins test for the return code of ssnprintf, e.g. nginx, postgresql, etc. so this is critical

[faxm0dem]

See #3153

[rpv-tomsk]

The reason is:

New function specification:

/* ssnprintf returns zero on success, one if truncation occurred
   and a negative integer onerror. */
int ssnprintf(char *str, size_t sz, const char *format, ...) {
  va_list ap;
  va_start(ap, format);

   int ret = vsnprintf(str, sz, format, ap);

   va_end(ap);

   if (ret < 0) {
    return ret;
  }
  return (size_t)ret >= sz;
} /* int ssnprintf */

Replaced function specification:

int snprintf (char *__s, size_t__n, const char *__fmt, ...)

Like sprintf(), but instead of assuming s to be of infinite size, 
no more than n characters (including the trailing NUL character) will be converted to s.

Returns the number of characters that would have been written to s if there were enough space.

Returns the number of characters that would have been written to s if there were enough space.

While new returns 0.

Sorry for missing this. I was in reviewers too, but has not checked that carefully enough (spent too much time fighting against Team new rules and checks).

Also affected plugin: src/threshold.c

Proposed patch:
-   if (ret < 0) {
-   return ret;
-  }
-  return (size_t)ret >= sz;
+ return ret;

But I don't know if this will recover GCC complaints...

[faxm0dem]

your proposed change fixes rrdtool. I'm checking if it fixes postgresql just to make sure.
@sunkuranganath will it still build on RHEL8?

[sunkuranganath]

@faxm0dem I dont have RHEL environment setup in our labs.
So have to defer your question to @mrunge

[mrunge]

Should be fixed by https://github.com/collectd/collectd/pull/3237

[zebity]

Hi @mrunge & @rpv-tomsk ,

been away on break and saw that introduction of ssnprintf wrapper, broke some other code, my apologies.

Code snippet for wrapper was taken from here: https://github.com/collectd/collectd/pull/2895#issuecomment-437062237

Also be aware that the same code snippet is in: https://github.com/collectd/collectd/blob/master/src/collectdctl.c only this time in the static function:

static int _ssnprintf(char *str, size_t sz, const char *format, ...);

The code for this is exactly as per the: ssnprintf(char *str, size_t sz, const char *format, ...);

This was done separately though as including /utils/common/common.h result in a whole new set of conflicting type errors.

So question is do we want to fix the code in collectdctl.c as well ?

Cheers,

Zebity.

[mrunge]

@zebity yes, we should fix everything.

[zebity]

Hi @mrunge I see that you have done back port from 5.9.2 into 5.9 but not back into master or 5.9.1 . I will take fork and apply fixes back into master for both /src/utils/common/common.c and src/collectdctl.c . Work now underway.

[mrunge]

Hi @zebity The fix I did was a cherry-pick from master, see
https://github.com/collectd/collectd/commit/c80ef3c943ed11f49d8d2c0440823d43ef0d7cd5

[zebity]

Hi @mrunge , yes saw that and to your point of "yes, we should fix everything" I have added new pull request (into master - https://github.com/collectd/collectd/pull/3283) which:

• also does cherry pick of fix,
• aligns collectdctl.c with this fix and
• adds ssnprintf2 function to allow for future code to check for truncation error, rather than ignore it.

I have now just done review of all closed pulls to check that no-one has submitted any updates that was already doing return result check as per original broken fix, as this code would then need to be remediated to use ssnprintf2 variation rather than fixed ssnprintf (with same return behaviour as original snprintf).

My code scan found no such cases. So believe we have now "fixed everything". Question is should we now back port this into 5.9.1 ?

I have not touched 5.9.1 as you have been driving all of the 5.9.x branches (thanks very much)

Right now default version 5.8.1 (https://github.com/collectd/collectd/blob/master/version-gen.sh) and debian/canonical packages are still broken relative to qemu / virt plugin misalignment, which was fixed way back here: https://github.com/collectd/collectd/issues/3143