__make_mdesc compiler warning

[morrone]

ldms.c is triggering the following warning for me:

ldms.c: In function ‘__make_mdesc’:
ldms.c:1457:2: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
  strncpy(vd->vd_name_unit, md->name, name_len);
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ldms.c:1456:13: note: length computed here
  name_len = strlen(md->name) + 1;
             ^~~~~~~~~~~~~~~~
ldms.c:1460:3: warning: ‘strncpy’ specified bound depends on the length of the source argument [-Wstringop-overflow=]
   strncpy(&vd->vd_name_unit[name_len], md->unit, unit_len);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
ldms.c:1459:14: note: length computed here
   unit_len = strlen(md->unit) + 1;
              ^~~~~~~~~~~~~~~~

This warning seems valid to me. There doesn't seem to be checking to make sure that the destination is large enough. But I don't understand the macro magic going on here. I am not clear on how one would know what the available size of the destination is to add that check.

[tom95858]

We've been going around and around on this with @baallan. We really don't want to be using strncpy in my opinion because it is much less efficient than strcpy; we added all this because a static analysis tool was complaining about the use of strcpy. Most people don't realize this but strncpy not only validates the length, but zero fills the resulting size if the source len is less than the target len.

[morrone]

Sure. I was going to change the length option of strncpy() to essentially:

    len = min(dest_len, src_len)

That does avoid the extra zeros.

But then I realized that vd_name_unit_len is set at the end of __make_mdesc, so I wasn't sure that I could trust it to have the a useful value when the function is entered. And walking up the call stack to see where that buffer comes from and the size allocated for vd_name_unit, I hit magic-macro-land and abandoned ship. :)

[baallan]

One good reason for the strncpy behavior is the case where the entire buffer (including the 'excessively zeroed' tail) is going to go over the wire and otherwise expose data that potentially should not be exposed.

When we are better at keeping track of input string lengths and calling strlen on the unknown string piece exactly once (which tends to expand argument lists with lengths), we can/should of course safely call strcpy and any decent audit tool will be able to prove to itself (across function call boundaries) that we are ok and have earned the improved performance. Fortify is one such audit tool; i cannot vouch for the accuracy of recent clang or recent gcc string audits.

[tom95858]

@baallan, @morrone strncpy only applies to byte_array metric types, not other primitive array types such as double, uint64, etc... Initially all set memory is bzero'd, however, there is the case where set memory is re-used when it is deleted and it's underlying set memory is reused by another set with potentially different uid/gid. Currently only the set's meta-data portion of the metric set is bzero'd; therefore, the data portion of a metric set could potentially contain the contents of a previous metric set. I think the correct fix is to bzero both the meta-data and the data portion of a metric set in ldms_set_create. In that case, the data portion is initialized and there is no danger of accidentally exposing metric data from another metric set owned by a different user. In any event, we are not talking about the classic ploy of getting stack data on the wire.

[tom95858]

@baallan, @morrone please reopen as required.

[baallan]

I'll accept @tom95858 proposed bzero solution in set_create, but afaict on top of tree it has not been implemented. It's possible i'm missing some equivalent in the code....