rev: defensive null check for fp

[awmrozek_hotmail.com]

Summary

Add a defensive null check for 'fp' in rev.c to silence a false-positive static analyzer warning (core.NullDereference).

• fp is either stdin or the result of a successful fopen
• Static analyzer warns of possible null dereference
• Behavior does not change
• Tiny, safe improvement

Test Plan

Build and run rev:

$ ./rev /etc/passwd

outputs reversed lines as usual

$ ./rev missingfile

prints warning and exits with code 1

Behavior is unchanged; static analyzer warning is silenced.

[fuz]

Could you show me an example where line 86 is reached while fp is a null pointer?

For this to happen, fp needs to be a null pointer at the beginning of the iteration, for it is only assigned to in line 71 and if the assignment results in a null pointer, the rest of the iteration is skipped.
But if fp was a null pointer at the beginning of the iteration, it only stays a null pointer if *argv is a null pointer, as otherwise the if statement in line 70 either makes fp not null or skips the rest of the iteration.
If *argv is a null pointer, it cannot be the first iteration, as fp = stdin in the first iteration. But it can also not be a subsequent iteration, as *argv being a null pointer is the exit condition of the loop.

So I don't think this is actually a case that can happen. And in any case, just silently ignoring this possibility seems wrong if it could happen.

[awmrozek_hotmail.com]

Hi!

make analyze in /usr/src/usr.bin/rev gave me the following:

/usr/src/usr.bin/rev/rev.c:86:7: warning: Access to field '_flags' results in a dereference of a null pointer (loaded from variable 'fp') [core.NullDereference]

I thought it could be nice for my first patch :-)

//welw

[awmrozek_hotmail.com]

make analyze of course, not make clean

[fuz]

I believe that, but it seems to be a false positive.
Please check if the condition can actually occur in practice.

[awmrozek_hotmail.com]

I'm sorry, I was a bit unclear what I meant. Here is a more refined version:

1. No, unfortunately I am not able to provide any path that leads to this condition. My claims are blindly based on first comment that make analyze returned. I will return to this commit if I ever manage to satisfy this condition.
2. I appreciate your time taking time and reading my commit, thanks so much! I was very curious about the review process itself, so I decided to commit something very simple. Thanks again!

[fuz]

Don't worry about it!

I think there is still room for improvement as the control flow of this tool is very hard to understand, but as there are no known bugs, it might be better to leave it as is.
Please also think about the consequences of your fix. What would happen here if fp is a null pointer and thus the branch in line 86 is not taken. Is this really the desired behaviour? Try to think about that! Getting a hint from a static analysis tool is only the first step, the real work only starts there.