Fun and games with, er, test coverage

[ewx]

Here's some gcov output:

       34:  599:  switch(pflags & (SSH_FXF_CREAT|SSH_FXF_TRUNC|SSH_FXF_EXCL)) {
        -:  600:  case 0:
       11:  601:    flags |= SSH_FXF_OPEN_EXISTING;
       11:  602:    break;
        -:  603:  case SSH_FXF_TRUNC:
        -:  604:    /* The drafts demand that SSH_FXF_CREAT also be sent making this formally
        -:  605:     * invalid, though there doesn't seem any good reason for them to do so:
        -:  606:     * the client intent seems clear.*/
        2:  607:    flags |= SSH_FXF_TRUNCATE_EXISTING;
        2:  608:    break;
        -:  609:  case SSH_FXF_CREAT:
        2:  610:    flags |= SSH_FXF_OPEN_OR_CREATE;
        2:  611:    break;
        -:  612:  case SSH_FXF_CREAT|SSH_FXF_TRUNC:
       17:  613:    flags |= SSH_FXF_CREATE_TRUNCATE;
       17:  614:    break;
        -:  615:  case SSH_FXF_CREAT|SSH_FXF_EXCL:
        -:  616:  case SSH_FXF_CREAT|SSH_FXF_TRUNC|SSH_FXF_EXCL: /* nonsensical */
    #####:  617:    flags |= SSH_FXF_CREATE_NEW;
    #####:  618:    break;
        -:  619:  default:
    #####:  620:    return SSH_FX_BAD_MESSAGE;
        -:  621:  }

Why are lines 617 and 618 never executed (that's what the hash signs mean)? (If you saw the answer on IRC then you're not eligible l-) Here's a couple of wrong answers:

• Because the tests don't exercise that case. They do, via SSH_FXF_CREAT|SSH_FXF_EXCL.
• Because you turned optimization on, despite the gcov manual telling you not to. Nope, -O0 and other options exactly as required.

(620 isn't executed because the tests don't go there. Apart from the code being obviously correct, no working client is going to send the server there anyway, so I don't really care.)

Comments

[simont]

<spoiler>Because SSH_FXF_CREATE_NEW is equal to zero, so the |= operation is null and the switch simply doesn't bother to do anything at all in that case?</spoiler>

[gerald_duck]

I'm guessing it could happen if the values of the various SSH_FXF_* macros were bogus — i.e. not bitwise independent — but I can't offhand see a way that wouldn't produde a compiler error.

And surely in security-critical code you have to test cases no working client is going to exercise, in case a malicious client decides to have a go?

[fanf]

Perhaps _EXCL == 0

I'm not sure _CREATE_NEW == 0 being the cause is consistent with -O0. Another possibility, that flags already has the _CREATE_NEW bit(s) set, would also require optimization for the compiler to discard the code.