Ueventd: Fix a corner case in ReadUevent() that triggers duplicate firmware loading.
Presently, within ReadUevent(), true is returned for a successful case as well as for the case where we read an invalid uevent (overflowed buffer)where the Uevent object is not cleared, and the caller calls the callback (with the earlier stored uevent object),leading to duplicate firmware loading. Uevent uevent; while (ReadUevent(&uevent)) { if (callback(uevent) == ListenerAction::kStop) return; } Scenario: 1. Proper Uevent received and callback is called (firmware loading is triggered). 2. Overflowed uevent is received as part of the same ReadUevent session, ReadUevent() returns true, but the uevent object is not cleared and still has earlier event values. 3. Callback is called again, leading to duplicate firmware load. Handle it by adding explicit return codes to let the caller know if the uevent read is invalid, and the caller can ignore it and read further pending uevents. Bug: 161580785 Change-Id: I09e80052337fd1495b968dc02ecff5ceb683da18
This commit is contained in:
parent
af7c3d5873
commit
a86bf05a01
2 changed files with 19 additions and 9 deletions
|
@ -95,20 +95,18 @@ UeventListener::UeventListener(size_t uevent_socket_rcvbuf_size) {
|
|||
fcntl(device_fd_, F_SETFL, O_NONBLOCK);
|
||||
}
|
||||
|
||||
bool UeventListener::ReadUevent(Uevent* uevent) const {
|
||||
ReadUeventResult UeventListener::ReadUevent(Uevent* uevent) const {
|
||||
char msg[UEVENT_MSG_LEN + 2];
|
||||
int n = uevent_kernel_multicast_recv(device_fd_, msg, UEVENT_MSG_LEN);
|
||||
if (n <= 0) {
|
||||
if (errno != EAGAIN && errno != EWOULDBLOCK) {
|
||||
PLOG(ERROR) << "Error reading from Uevent Fd";
|
||||
}
|
||||
return false;
|
||||
return ReadUeventResult::kFailed;
|
||||
}
|
||||
if (n >= UEVENT_MSG_LEN) {
|
||||
LOG(ERROR) << "Uevent overflowed buffer, discarding";
|
||||
// Return true here even if we discard as we may have more uevents pending and we
|
||||
// want to keep processing them.
|
||||
return true;
|
||||
return ReadUeventResult::kInvalid;
|
||||
}
|
||||
|
||||
msg[n] = '\0';
|
||||
|
@ -116,7 +114,7 @@ bool UeventListener::ReadUevent(Uevent* uevent) const {
|
|||
|
||||
ParseEvent(msg, uevent);
|
||||
|
||||
return true;
|
||||
return ReadUeventResult::kSuccess;
|
||||
}
|
||||
|
||||
// RegenerateUevents*() walks parts of the /sys tree and pokes the uevent files to cause the kernel
|
||||
|
@ -137,7 +135,10 @@ ListenerAction UeventListener::RegenerateUeventsForDir(DIR* d,
|
|||
close(fd);
|
||||
|
||||
Uevent uevent;
|
||||
while (ReadUevent(&uevent)) {
|
||||
ReadUeventResult result;
|
||||
while ((result = ReadUevent(&uevent)) != ReadUeventResult::kFailed) {
|
||||
// Skip processing the uevent if it is invalid.
|
||||
if (result == ReadUeventResult::kInvalid) continue;
|
||||
if (callback(uevent) == ListenerAction::kStop) return ListenerAction::kStop;
|
||||
}
|
||||
}
|
||||
|
@ -212,7 +213,10 @@ void UeventListener::Poll(const ListenerCallback& callback,
|
|||
// We're non-blocking, so if we receive a poll event keep processing until
|
||||
// we have exhausted all uevent messages.
|
||||
Uevent uevent;
|
||||
while (ReadUevent(&uevent)) {
|
||||
ReadUeventResult result;
|
||||
while ((result = ReadUevent(&uevent)) != ReadUeventResult::kFailed) {
|
||||
// Skip processing the uevent if it is invalid.
|
||||
if (result == ReadUeventResult::kInvalid) continue;
|
||||
if (callback(uevent) == ListenerAction::kStop) return;
|
||||
}
|
||||
}
|
||||
|
|
|
@ -37,6 +37,12 @@ enum class ListenerAction {
|
|||
kContinue, // Continue regenerating uevents as we haven't seen the one(s) we're interested in.
|
||||
};
|
||||
|
||||
enum class ReadUeventResult {
|
||||
kSuccess = 0, // Uevent was successfully read.
|
||||
kFailed, // Uevent reading has failed.
|
||||
kInvalid, // An Invalid Uevent was read (like say, the msg received is >= UEVENT_MSG_LEN).
|
||||
};
|
||||
|
||||
using ListenerCallback = std::function<ListenerAction(const Uevent&)>;
|
||||
|
||||
class UeventListener {
|
||||
|
@ -50,7 +56,7 @@ class UeventListener {
|
|||
const std::optional<std::chrono::milliseconds> relative_timeout = {}) const;
|
||||
|
||||
private:
|
||||
bool ReadUevent(Uevent* uevent) const;
|
||||
ReadUeventResult ReadUevent(Uevent* uevent) const;
|
||||
ListenerAction RegenerateUeventsForDir(DIR* d, const ListenerCallback& callback) const;
|
||||
|
||||
android::base::unique_fd device_fd_;
|
||||
|
|
Loading…
Reference in a new issue