PR#35: lnewusers: flose(fp) to avoid a Coverity warning - libuser

libuser

#35 lnewusers: flose(fp) to avoid a Coverity warning

Closed 2 years ago by thalman. Opened 5 years ago by jhrozek.

jhrozek/libuser lnewuser_fclose into master

lnewusers: flose(fp) to avoid a Coverity warning

Jakub Hrozek • 5 years ago

0210ee4

apps/lnewusers.c

file modified

		`@@ -305,6 +305,10 @@`
		`lu_ent_clear_all(groupEnt);`
		`}`

		`+ if (fp) {`
		`+ fclose(fp);`
		`+ }`
		`+`
		`lu_ent_free(groupEnt);`
		`lu_ent_free(ent);`

jhrozek commented 5 years ago

Merges:
https://pagure.io/libuser/issue/20

Please note that this patch also ADDS a Coverity warning:

libuser-0.62/apps/lnewusers.c:97: deref_ptr_in_call: Dereferencing pointer "fp". [Note: The source code implementation of the function has been overridden by a builtin model.]
libuser-0.62/apps/lnewusers.c:306: check_after_deref: Null-checking "fp" suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
#  304|     }
#  305|   
#  306|->         if (fp) {
#  307|               fclose(fp);
#  308|           }

But I think checking a file handle validity is a good defensive behaviour.

mitr commented 5 years ago

fclose(stdin) causes undefined behavior, see http://pubs.opengroup.org/onlinepubs/9699919799/functions/fclose.html ,

(My personal taste would be not to add “defensive” tests that can’t happen, but that’s neither here nor there; it’s your project now.)

jhrozek commented 5 years ago

Yes, I tend to agree.

I'm actually tempted to just close this PR and the issue. This is a short-running process after all, not a daemon.

mitr commented 5 years ago

Or, well, be precise, and have a FILE *file_to_close, which is NULL if input is from stdin, and non-NULL if it is another handle. That might confuse Coverity even more, though…