Fix double-close in EventConverterEvdevImpl

Fix double-close in EventConverterEvdevImpl

This fixes a double close that occurs when a keyboard or mouse is
unplugged from the system, which will cause breakage if there is a
concurrent open(). It also switches to a scoped file descriptor wrapper,
with a special close function because close() can return ENODEV on an
input device that was unplugged.

BUG=681865, 660960
TEST=events_unittests

Review-Url: https://codereview.chromium.org/2639043002
Cr-Commit-Position: refs/heads/master@{#444544}
Committed: https://chromium.googlesource.com/chromium/src/+/13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1

[spang, 2017-01-17 20:28:47]

spang@chromium.org changed reviewers:
+ davidben@chromium.org

[spang, 2017-01-17 20:28:47]

I am not really sure about forking ScopedFD, but it's clear that the current
implementation is too strict on the Linux platform. Even if close() returns an
error on linux, the file descriptor is still closed.

Maybe we should change ScopedFD?

[spang, 2017-01-17 20:39:53]

Hm, I suppose we should really just fix ScopedFD. It seems like it would fix the
original bug 660960 as well.

[davidben, 2017-01-17 20:44:26]

On 2017/01/17 20:39:53, spang wrote:
> Hm, I suppose we should really just fix ScopedFD. It seems like it would fix
the
> original bug 660960 as well.

I don't believe that would fix 660960. EBADFD means something really bad
happened and we want to fix the root cause. The crash reports are showing
EBADFD, not ENODEV. If ENODEV can happen without us double-closing an fd or
something else crazy, then yeah it sounds like we should be ignoring that?

Though that sounds like a terrible thing for the kernel to be returning. If
userspace gets ENODEV, what does that mean for the fd? Is the still in the fd
table or not?

(close ~ free. You don't want free to return an error. Either the memory is not
a malloc'd pointer (EBADFD), in which case something terrible happened and you
should crash, or you should cleanly free it. If it returns failure, there isn't
much useful the caller can do. You don't want the caller to double-free or leak
memory.)

[davidben, 2017-01-17 20:54:23]

On 2017/01/17 20:44:26, davidben wrote:
> On 2017/01/17 20:39:53, spang wrote:
> > Hm, I suppose we should really just fix ScopedFD. It seems like it would fix
> the
> > original bug 660960 as well.
> 
> I don't believe that would fix 660960. EBADFD means something really bad
> happened and we want to fix the root cause. The crash reports are showing
> EBADFD, not ENODEV. If ENODEV can happen without us double-closing an fd or
> something else crazy, then yeah it sounds like we should be ignoring that?
> 
> Though that sounds like a terrible thing for the kernel to be returning. If
> userspace gets ENODEV, what does that mean for the fd? Is the still in the fd
> table or not?
> 
> (close ~ free. You don't want free to return an error. Either the memory is
not
> a malloc'd pointer (EBADFD), in which case something terrible happened and you
> should crash, or you should cleanly free it. If it returns failure, there
isn't
> much useful the caller can do. You don't want the caller to double-free or
leak
> memory.)

Looks like this was fixed in the kernel here:
https://github.com/torvalds/linux/commit/eb38f3a4f6e86f8bb10a3217ebd85ecc5d76...

This comment says ENODEV means the fd is removed from the fd table:
https://bugzilla.opensuse.org/show_bug.cgi?id=939571#c35

[spang, 2017-01-17 21:17:37]

On 2017/01/17 20:44:26, davidben wrote:
> On 2017/01/17 20:39:53, spang wrote:
> > Hm, I suppose we should really just fix ScopedFD. It seems like it would fix
> the
> > original bug 660960 as well.
> 
> I don't believe that would fix 660960. EBADFD means something really bad
> happened and we want to fix the root cause. The crash reports are showing
> EBADFD, not ENODEV. If ENODEV can happen without us double-closing an fd or
> something else crazy, then yeah it sounds like we should be ignoring that?
> 

Ok, if it's EBADF you're seeing then I agree, it indicates an error and relaxing
the check wouldn't have helped.

> Though that sounds like a terrible thing for the kernel to be returning. If
> userspace gets ENODEV, what does that mean for the fd? Is the still in the fd
> table or not?

It's closed.

> 
> (close ~ free. You don't want free to return an error. Either the memory is
not
> a malloc'd pointer (EBADFD), in which case something terrible happened and you
> should crash, or you should cleanly free it. If it returns failure, there
isn't
> much useful the caller can do. You don't want the caller to double-free or
leak
> memory.)

I agree in principle but the API include an error code and they can happen for
drivers that try to do some flushing work on close().

I'm not sure about other kernels but on Linux close() can return an error but
will not fail to remove the file descriptor, if it exists.

[davidben, 2017-01-17 21:23:34]

On 2017/01/17 21:17:37, spang wrote:
> On 2017/01/17 20:44:26, davidben wrote:
> > On 2017/01/17 20:39:53, spang wrote:
> > > Hm, I suppose we should really just fix ScopedFD. It seems like it would
fix
> > the
> > > original bug 660960 as well.
> > 
> > I don't believe that would fix 660960. EBADFD means something really bad
> > happened and we want to fix the root cause. The crash reports are showing
> > EBADFD, not ENODEV. If ENODEV can happen without us double-closing an fd or
> > something else crazy, then yeah it sounds like we should be ignoring that?
> > 
> 
> Ok, if it's EBADF you're seeing then I agree, it indicates an error and
relaxing
> the check wouldn't have helped.
> 
> > Though that sounds like a terrible thing for the kernel to be returning. If
> > userspace gets ENODEV, what does that mean for the fd? Is the still in the
fd
> > table or not?
> 
> It's closed.
> 
> > 
> > (close ~ free. You don't want free to return an error. Either the memory is
> not
> > a malloc'd pointer (EBADFD), in which case something terrible happened and
you
> > should crash, or you should cleanly free it. If it returns failure, there
> isn't
> > much useful the caller can do. You don't want the caller to double-free or
> leak
> > memory.)
> 
> I agree in principle but the API include an error code and they can happen for
> drivers that try to do some flushing work on close().
> 
> I'm not sure about other kernels but on Linux close() can return an error but
> will not fail to remove the file descriptor, if it exists.

Right. I think it's reasonable to modify ScopedFD to only crash on EBADF. The
kernel doing that is Bad Manners and probably should be a bug (looks like
they've considered it a bug before judging by the link). But that doesn't change
the fact that older Linux kernels did that and I'm sure there's other codepaths,
so only crashing on EBADF is probably best.

Though I'm also not the person who would have to approve such a change.

[spang, 2017-01-17 22:05:27]

On 2017/01/17 21:23:34, davidben wrote:
> On 2017/01/17 21:17:37, spang wrote:
> > On 2017/01/17 20:44:26, davidben wrote:
> > > On 2017/01/17 20:39:53, spang wrote:
> > > > Hm, I suppose we should really just fix ScopedFD. It seems like it would
> fix
> > > the
> > > > original bug 660960 as well.
> > > 
> > > I don't believe that would fix 660960. EBADFD means something really bad
> > > happened and we want to fix the root cause. The crash reports are showing
> > > EBADFD, not ENODEV. If ENODEV can happen without us double-closing an fd
or
> > > something else crazy, then yeah it sounds like we should be ignoring that?
> > > 
> > 
> > Ok, if it's EBADF you're seeing then I agree, it indicates an error and
> relaxing
> > the check wouldn't have helped.
> > 
> > > Though that sounds like a terrible thing for the kernel to be returning.
If
> > > userspace gets ENODEV, what does that mean for the fd? Is the still in the
> fd
> > > table or not?
> > 
> > It's closed.
> > 
> > > 
> > > (close ~ free. You don't want free to return an error. Either the memory
is
> > not
> > > a malloc'd pointer (EBADFD), in which case something terrible happened and
> you
> > > should crash, or you should cleanly free it. If it returns failure, there
> > isn't
> > > much useful the caller can do. You don't want the caller to double-free or
> > leak
> > > memory.)
> > 
> > I agree in principle but the API include an error code and they can happen
for
> > drivers that try to do some flushing work on close().
> > 
> > I'm not sure about other kernels but on Linux close() can return an error
but
> > will not fail to remove the file descriptor, if it exists.
> 
> Right. I think it's reasonable to modify ScopedFD to only crash on EBADF. The
> kernel doing that is Bad Manners and probably should be a bug (looks like
> they've considered it a bug before judging by the link). But that doesn't
change
> the fact that older Linux kernels did that and I'm sure there's other
codepaths,
> so only crashing on EBADF is probably best.
> 
> Though I'm also not the person who would have to approve such a change.

I've sent out a separate patch. If approved, I will change this patch to use
ScopedFD directly.

[davidben, 2017-01-18 19:48:10]

lgtm modulo the ScopedFD vs ScopedInputDevice question which is getting resolved
in the other CL.

https://codereview.chromium.org/2639043002/diff/1/ui/events/ozone/evdev/scope...
File ui/events/ozone/evdev/scoped_input_device.cc (right):

https://codereview.chromium.org/2639043002/diff/1/ui/events/ozone/evdev/scope...
ui/events/ozone/evdev/scoped_input_device.cc:32:
base::debug::Alias(&close_errno);
(Same comment as in the other CL that this base::debug::Alias isn't really
necessary with the modified PCHECK.)

[spang, 2017-01-18 20:40:35]

https://codereview.chromium.org/2639043002/diff/1/ui/events/ozone/evdev/scope...
File ui/events/ozone/evdev/scoped_input_device.cc (right):

https://codereview.chromium.org/2639043002/diff/1/ui/events/ozone/evdev/scope...
ui/events/ozone/evdev/scoped_input_device.cc:32:
base::debug::Alias(&close_errno);
On 2017/01/18 19:48:10, davidben wrote:
> (Same comment as in the other CL that this base::debug::Alias isn't really
> necessary with the modified PCHECK.)

done.

[spang, 2017-01-18 20:40:51]

The CQ bit was checked by spang@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-18 20:41:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[spang, 2017-01-18 20:57:57]

Description was changed from

==========
Fix double-close in EventConverterEvdevImpl

This fixes a double close that occurs when a keyboard or mouse is
unplugged from the system, which will cause breakage if there is a
concurrent open(). It also switches to a scoped file descriptor wrapper,
with a special close function because close() can return ENODEV on an
input device that was unplugged.

BUG=681865
TEST=events_unittests
==========

to

==========
Fix double-close in EventConverterEvdevImpl

This fixes a double close that occurs when a keyboard or mouse is
unplugged from the system, which will cause breakage if there is a
concurrent open(). It also switches to a scoped file descriptor wrapper,
with a special close function because close() can return ENODEV on an
input device that was unplugged.

BUG=681865, 660960
TEST=events_unittests
==========

[spang, 2017-01-18 20:59:04]

spang@chromium.org changed reviewers:
+ kpschoedel@chromium.org

[spang, 2017-01-18 21:01:25]

spang@chromium.org changed reviewers:
+ xiyuan@chromium.org

[kpschoedel, 2017-01-18 21:31:07]

LGTM

https://codereview.chromium.org/2639043002/diff/20001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc
(right):

https://codereview.chromium.org/2639043002/diff/20001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc:56:
evdev_.fd = fd.release();
evdev_ owns the fd now, and EvdevClose closes it?

[xiyuan, 2017-01-18 21:42:39]

lgtm

A plus would be use ScopedFD when the other CL is approved.

Thank you for the quick fix. Appreciated.

[spang, 2017-01-18 22:05:37]

https://codereview.chromium.org/2639043002/diff/20001/ui/events/ozone/evdev/l...
File ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc
(right):

https://codereview.chromium.org/2639043002/diff/20001/ui/events/ozone/evdev/l...
ui/events/ozone/evdev/libgestures_glue/event_reader_libevdev_cros.cc:56:
evdev_.fd = fd.release();
On 2017/01/18 21:31:07, kpschoedel wrote:
> evdev_ owns the fd now, and EvdevClose closes it?

Correct.
https://cs.corp.google.com/chromeos_public/src/platform/libevdev/src/libevdev...

[commit-bot: I haz the power, 2017-01-18 22:59:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-18 22:59:25]

Dry run: This issue passed the CQ dry run.

[spang, 2017-01-18 23:29:51]

The CQ bit was checked by spang@chromium.org

[spang, 2017-01-18 23:29:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/2639043002/#ps20001
(title: "remove base::debug::Alias")

[commit-bot: I haz the power, 2017-01-18 23:30:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-18 23:36:13]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484782191314230,
"parent_rev": "57b9ce859ef097374f8ed46df3ffa66eefa0f243", "commit_rev":
"13ad1ff07b21ed9f4b7b5cb7c7cebdff0b8fe0c1"}

[commit-bot: I haz the power, 2017-01-18 23:36:52]

Description was changed from

==========
Fix double-close in EventConverterEvdevImpl

This fixes a double close that occurs when a keyboard or mouse is
unplugged from the system, which will cause breakage if there is a
concurrent open(). It also switches to a scoped file descriptor wrapper,
with a special close function because close() can return ENODEV on an
input device that was unplugged.

BUG=681865, 660960
TEST=events_unittests
==========

to

==========
Fix double-close in EventConverterEvdevImpl

This fixes a double close that occurs when a keyboard or mouse is
unplugged from the system, which will cause breakage if there is a
concurrent open(). It also switches to a scoped file descriptor wrapper,
with a special close function because close() can return ENODEV on an
input device that was unplugged.

BUG=681865, 660960
TEST=events_unittests

Review-Url: https://codereview.chromium.org/2639043002
Cr-Commit-Position: refs/heads/master@{#444544}
Committed:
https://chromium.googlesource.com/chromium/src/+/13ad1ff07b21ed9f4b7b5cb7c7ce...
==========

[commit-bot: I haz the power, 2017-01-18 23:36:59]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/13ad1ff07b21ed9f4b7b5cb7c7ce...

[satorux1, 2017-01-20 02:06:31]

satorux@chromium.org changed reviewers:
+ satorux@chromium.org

[satorux1, 2017-01-20 02:06:32]

https://codereview.chromium.org/2639043002/diff/20001/ui/events/ozone/evdev/s...
File ui/events/ozone/evdev/scoped_input_device.cc (right):

https://codereview.chromium.org/2639043002/diff/20001/ui/events/ozone/evdev/s...
ui/events/ozone/evdev/scoped_input_device.cc:27: PCHECK(0 == ret || errno !=
EBADF);
dribe-by: maybe add a comment explaining why EBADF is allowed?

[satorux1, 2017-01-20 02:10:36]

https://codereview.chromium.org/2639043002/diff/20001/ui/events/ozone/evdev/s...
File ui/events/ozone/evdev/scoped_input_device.cc (right):

https://codereview.chromium.org/2639043002/diff/20001/ui/events/ozone/evdev/s...
ui/events/ozone/evdev/scoped_input_device.cc:27: PCHECK(0 == ret || errno !=
EBADF);
On 2017/01/20 02:06:32, satorux1 wrote:
> dribe-by: maybe add a comment explaining why EBADF is allowed?

my bad. errors other than EBADF are allowed.