[CCing msebor and amacleod for -Wformat-truncation and gcc value-range expertise]

Building mdadm-4.0-6.fc29.src.rpm with gcc-8.0.1-0.16.fc29.x86_64 I see just these two fatal -Werror warnings:

mdopen.c: In function ‘make_parts’:
mdopen.c:93:38: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
    snprintf(sym, sizeof(sym), "%s%s%d", orig, odig?"p":"", i);
                                      ^
mdopen.c:93:4: note: ‘snprintf’ output 2 or more bytes (assuming 1025) into a destination of size 1024
    snprintf(sym, sizeof(sym), "%s%s%d", orig, odig?"p":"", i);
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

super-intel.c: In function ‘imsm_process_update’:
super-intel.c:9355:58: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
   snprintf((char *) dev->volume, MAX_RAID_SERIAL_LEN, "%s", name);
                                                          ^
super-intel.c:9355:3: note: ‘snprintf’ output between 1 and 17 bytes into a destination of size 16
   snprintf((char *) dev->volume, MAX_RAID_SERIAL_LEN, "%s", name);
   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

(I don't see the Monitor.c and super-ddf.c issues from the log in comment #0).

The warning in mdopen.c: make_parts appears to be a false positive, but gcc would need smarter value-range tracking, plus knowledge of the semantics of readlink to fix it; it's essentially:

  char orig[1024];
  char sym[1024];

  if (cond) {
    int len = readlink(dev, orig, sizeof(orig));
    if (len < 0 || len > 1000)
      return;
  }

  /* various code */

  if (cond, re-evaluated, but can't change) {
    snprintf(sym, sizeof(sym), "%s%s%d", orig, odig?"p":"", i);
  }

GCC doesn't "know" of the relationship between strlen(orig) and "len" from the "readlink" call, and hence doesn't know that at the snprintf call that strlen(orig) <= 1000.  Also, the relationship only holds when "cond" holds both times.

Martin, Andrew: is this something that's in-scope to fix in gcc?  (AFAIK, the value-range code would need to be able to handle the conditions on the value-range, which is, I think beyond what we currently do)


The warning in super-intel.c: imsm_process_update also looks like a false positive; the code looks like:

  char name[MAX_RAID_SERIAL_LEN+1];

  snprintf(name, MAX_RAID_SERIAL_LEN, "%s", (char *) u->name);
  name[MAX_RAID_SERIAL_LEN] = '\0';

  /* other code */	    

  snprintf((char *) dev->volume, MAX_RAID_SERIAL_LEN, "%s", name);

Hence we ought to "know" that strlen(name) <= MAX_RAID_SERIAL_LEN, and thus, AIUI, presumably we ought not to warn for this.  Martin?


-Wformat-truncation was added in gcc 7; I'm not sure why you didn't see these warnings before (could be the code changed, or the compiler got getter at detecting the issue).

In the first case, the warning doesn't know the relationship between readlink(dev, orig, sizeof orig) and strlen(orig) so it assumes the latter can be as much as 1023.  To avoid the warning either specify precision in the first %s directive: %.1000s, or handle the (hypothetical) truncation, or use sprintf (snprintf is unnecessary here since the readlink result guarantees that truncation or overflow don't happen.  In GCC 9 we can look at making it possible to establish a relationship between calls like readlink() and the lengths of strings they create.

In the second case, if the type of u->name is char[MAX_RAID_SERIAL_LEN + 1] then its max length is MAX_RAID_SERIAL_LEN and the snprintf call could truncate the last character of the string by setting u->name[MAX_RAID_SERIAL_LEN - 1] to nul.  snprintf should be called with the size of the buffer as the second argument.

(In reply to Martin Sebor from comment #2)

Thanks.

> In the first case, the warning doesn't know the relationship between
> readlink(dev, orig, sizeof orig) and strlen(orig) so it assumes the latter
> can be as much as 1023.  To avoid the warning either specify precision in
> the first %s directive: %.1000s, or handle the (hypothetical) truncation, or
> use sprintf (snprintf is unnecessary here since the readlink result
> guarantees that truncation or overflow don't happen.  In GCC 9 we can look
> at making it possible to establish a relationship between calls like
> readlink() and the lengths of strings they create.

That would improve some cases, but probably wouldn't help for this case, due to the conditions; it's something like:

  char orig[1024];
  char sym[1024];

  if (some_struct.flag) {
    int len = readlink(dev, orig, sizeof(orig));
    if (len < 0 || len > 1000)
      return;
  }

  /* various code, which doesn't touch some_struct */

  if (some_struct.flag) {
    snprintf(sym, sizeof(sym), "%s%s%d", orig, odig?"p":"", i);
  }

so at the snprintf call you only "know" that len == strlen(orig) if you can prove that you came from the readlink call, which requires that knowledge that "some_struct.flag" being true at the second conditional means that it was true at the first conditional, which AFAIK requires something more sophisticated than just walking the dominator tree.

> In the second case, if the type of u->name is char[MAX_RAID_SERIAL_LEN + 1]
> then its max length is MAX_RAID_SERIAL_LEN and the snprintf call could
> truncate the last character of the string by setting
> u->name[MAX_RAID_SERIAL_LEN - 1] to nul.  snprintf should be called with the
> size of the buffer as the second argument.

So this one is a true positive.

Thanks again.

There are a lot of things at play in this testcase.  Lets look at the best case:

if the middle code is not significant, jump threading could decide to connect the 2 blocks if it was identified as worthwhile, and then we'd be able to figure out the range for len pretty easily.  It would still need some way to connect the fact that len is related to the length or orig somehow... perhaps that is what martins code would/can do.  My guess is there is too much code in the middle, or the threader just doesn't see anything of value. 

If we fail the best case scenario, then we have the unfortunate situation that len goes out of scope, and thus any time after the first block, we know nothing about the range. 

To the compiler its not that big a deal since scopes are faked anyway with a function local, but after that block, we have to revert to knowing nothing about the range of len since it could also NOT have taken that path and it'll have some random garbage value from the start of the function.  


If it were rewritten slightly instead to be:

  char orig[1024];
  char sym[1024];
  int len = 0;

  if (some_struct.flag) {
    len = readlink(dev, orig, sizeof(orig));
    if (len < 0 || len > 1000)
      return;
  }

  /* various code, which doesn't touch some_struct */

  if (some_struct.flag) {
    snprintf(sym, sizeof(sym), "%s%s%d", orig, odig?"p":"", i);
  }

Then the new range code I am working on would known that len was [0,1000] in the second block (I tested it to see :-)   And maybe the current code would too, I'm not sure. 

But you'd still need to associate len with strlen(orig) somehow.

And then the worst case is no change to the code... the problem then becomes that you need to track states and ranges that are dependent on other states..  ie, we know a range for len only when we know (some_struct.flag) is true.  This is bordering on some sort of predicate analysis that I do not believe we are equipped to deal with yet.

It is conceivable that an industrious pass could identify blocks that share conditions and look up things it cares about in the other block, but that seems like a lot of work in the general case.

Do those 2 IF's share the same ssa-name in the condition?   ie, does the compiler know that they are the same condition?  if it does, I suppose you might be able to figure something out if you can identify one block contains something of interest to you in the other block. 

Remind me of this in about 6 months.  I'm going to think about whats involved in "path pruning" when resolving ranges.  ie, when you ask for the range of 'len' in that second block, we look back through the previous blocks to see what is known about 'len', but we don't carry back "unrelated" conditions along that path. Too many could accumulate. 

In theory (and this one is a simple case), we'd note that (some_struct.flag) must be true where-ever we look, and if we see a branch with the same guard, we would not evaluate the path that couldn't have been taken.  

Again, In theory this would then be able to tell you len is [0,1000] in the second block if you can determine you need to know that.  Im not sure that would fall into the general case or not... It may be expensive to carry all these conditions all the time, and usually they are not needed.  I'm simply not sure.    If it doesn't, then I think it could be provided under one of the alternate APIs.  

But we're not there yet. Hit me in September if it is useful.

I was thinking of associating len with readlink() in a way that would let the strlen pass deduce strlen(orig) from the 'if (len < 0 || len > 1000) return;' statement.  Something like a function attribute returns_strlen(argno):

  extern __attribute__ ((returns_strlen (2))) int
  readlink (int, char*, size_t);

Once it saw a call to readlink() (or any other function declared with the returns_strlen attribute), the strlen pass would look for the use of the return value closest to the use of the corresponding readlink() argument where its length is needed and use the return value's range to record the argument's length.  This should work regardless of whether or not len stayed in scope.

I should also add that GCC 8 got better about determining string length ranges from unknown strings (it uses the size of the array the string is stored in to compute the upper bound of the length).  This improvement has in turn enabled the warning for cases where it was previously disabled.

gcc-8 fixes are now in the git repo: https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/log/

mdadm-4.1-rc2.0.3.fc30 has been submitted as an update to Fedora 30. https://bodhi.fedoraproject.org/updates/FEDORA-2019-5111be3e77

mdadm-4.1-rc2.0.3.fc30 has been pushed to the Fedora 30 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2019-5111be3e77

mdadm-4.1-rc2.0.3.fc30 has been pushed to the Fedora 30 stable repository. If problems still persist, please make note of it in this bug report.