On 2/19/19 7:50 PM, Dave Chinner wrote:
> On Tue, Feb 19, 2019 at 05:23:38PM -0600, Eric Sandeen wrote:
>> openfile() stats a path to determine whether it is a pipe, and then
>> opens it with flags based on the type it saw during the stat. It's
>> possible that the file at that path changes in between, and Coverity
>> points this out.
>>
>> Instead, always open O_NONBLOCK, stat the fd we got, and turn the
>> flag back off via fcntl() if it's not a pipe.
>>
>> Addresses-Coverity-ID: 1442788 ("Time of check time of use")
>
> For O_NONBLOCK I think there is zero harm in the code as it stands,
> but I don't really care about it tht much.
>
> I do wonder what happens if need to conditionally use O_NONBLOCK on
> open(), though....
>
>> @@ -112,6 +106,22 @@ openfile(
>> }
>> }
>>
>> + if (fstat(fd, &st) < 0) {
>> + perror("stat");
>> + close(fd);
>> + return -1;
>> + }
>> +
>> + /* We only want to keep nonblocking behavior for pipes */
>> + if (!S_ISFIFO(st.st_mode)) {
>> + oflags &= ~O_NONBLOCK;
>> + if (fcntl(fd, F_SETFL, oflags) < 0) {
>
> Don't you need to do oflags = fcntl(fd, F_GETFL, NULL) first?
maybe? We just opened it with oflags, but *shrug* I guess it doesn't
hurt?
>
>> + perror("fcntl");
>> + close(fd);
>> + return -1;
>> + }
>
> can you add a "goto out_close;" error jump to this function now that
> this is the fourth and fifth places that have this same close/return
> on error...
yeah, good point.
> CHeers,
>
> Dave.
>
