From d954486023aea49be22b53495166f40a028fdb67 Mon Sep 17 00:00:00 2001 From: "tao.pei" Date: Fri, 17 Jul 2015 17:18:41 +0800 Subject: Use errno correctly. After a failed write() or ioctl(), errno wasn't being checked until after some other function calls that could also modify errno, thus checking the wrong errno. Make sure to check it prior to doing anything else that can modify it. [Preconditions] 1.PC connects with phone(mtp). [Procedures] 1.Copy a file from PC to phone. 2.Cancel the copying. 3.Recopy the file. Change-Id: Id772fca7ccb96d3f43bd4beb210bedd8d3ac17fa --- media/mtp/MtpServer.cpp | 43 +++++++++++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 14 deletions(-) (limited to 'media/mtp/MtpServer.cpp') diff --git a/media/mtp/MtpServer.cpp b/media/mtp/MtpServer.cpp index 6a334e6..3c91ffd 100644 --- a/media/mtp/MtpServer.cpp +++ b/media/mtp/MtpServer.cpp @@ -215,10 +215,11 @@ void MtpServer::run() { mResponse.setTransactionID(transaction); ALOGV("sending response %04X", mResponse.getResponseCode()); ret = mResponse.write(fd); + const int savedErrno = errno; mResponse.dump(); if (ret < 0) { ALOGE("request write returned %d, errno: %d", ret, errno); - if (errno == ECANCELED) { + if (savedErrno == ECANCELED) { // return to top of loop and wait for next command continue; } @@ -793,15 +794,19 @@ MtpResponseCode MtpServer::doGetObject() { // then transfer the file int ret = ioctl(mFD, MTP_SEND_FILE_WITH_HEADER, (unsigned long)&mfr); - ALOGV("MTP_SEND_FILE_WITH_HEADER returned %d\n", ret); - close(mfr.fd); if (ret < 0) { - if (errno == ECANCELED) - return MTP_RESPONSE_TRANSACTION_CANCELLED; - else - return MTP_RESPONSE_GENERAL_ERROR; + if (errno == ECANCELED) { + result = MTP_RESPONSE_TRANSACTION_CANCELLED; + } else { + result = MTP_RESPONSE_GENERAL_ERROR; + } + } else { + result = MTP_RESPONSE_OK; } - return MTP_RESPONSE_OK; + + ALOGV("MTP_SEND_FILE_WITH_HEADER returned %d\n", ret); + close(mfr.fd); + return result; } MtpResponseCode MtpServer::doGetThumb() { @@ -870,14 +875,15 @@ MtpResponseCode MtpServer::doGetPartialObject(MtpOperationCode operation) { // transfer the file int ret = ioctl(mFD, MTP_SEND_FILE_WITH_HEADER, (unsigned long)&mfr); ALOGV("MTP_SEND_FILE_WITH_HEADER returned %d\n", ret); - close(mfr.fd); + result = MTP_RESPONSE_OK; if (ret < 0) { if (errno == ECANCELED) - return MTP_RESPONSE_TRANSACTION_CANCELLED; + result = MTP_RESPONSE_TRANSACTION_CANCELLED; else - return MTP_RESPONSE_GENERAL_ERROR; + result = MTP_RESPONSE_GENERAL_ERROR; } - return MTP_RESPONSE_OK; + close(mfr.fd); + return result; } MtpResponseCode MtpServer::doSendObjectInfo() { @@ -991,6 +997,7 @@ MtpResponseCode MtpServer::doSendObject() { MtpResponseCode result = MTP_RESPONSE_OK; mode_t mask; int ret, initialData; + bool isCanceled = false; if (mSendObjectHandle == kInvalidObjectHandle) { ALOGE("Expected SendObjectInfo before SendObject"); @@ -1038,6 +1045,10 @@ MtpResponseCode MtpServer::doSendObject() { ALOGV("receiving %s\n", (const char *)mSendObjectFilePath); // transfer the file ret = ioctl(mFD, MTP_RECEIVE_FILE, (unsigned long)&mfr); + if ((ret < 0) && (errno == ECANCELED)) { + isCanceled = true; + } + ALOGV("MTP_RECEIVE_FILE returned %d\n", ret); } } @@ -1045,7 +1056,7 @@ MtpResponseCode MtpServer::doSendObject() { if (ret < 0) { unlink(mSendObjectFilePath); - if (errno == ECANCELED) + if (isCanceled) result = MTP_RESPONSE_TRANSACTION_CANCELLED; else result = MTP_RESPONSE_GENERAL_ERROR; @@ -1214,6 +1225,7 @@ MtpResponseCode MtpServer::doSendPartialObject() { length -= initialData; } + bool isCanceled = false; if (ret < 0) { ALOGE("failed to write initial data"); } else { @@ -1225,12 +1237,15 @@ MtpResponseCode MtpServer::doSendPartialObject() { // transfer the file ret = ioctl(mFD, MTP_RECEIVE_FILE, (unsigned long)&mfr); + if ((ret < 0) && (errno == ECANCELED)) { + isCanceled = true; + } ALOGV("MTP_RECEIVE_FILE returned %d", ret); } } if (ret < 0) { mResponse.setParameter(1, 0); - if (errno == ECANCELED) + if (isCanceled) return MTP_RESPONSE_TRANSACTION_CANCELLED; else return MTP_RESPONSE_GENERAL_ERROR; -- cgit v1.1 From 9bdc0f69492d90fbceca68d3b40716b4722a3222 Mon Sep 17 00:00:00 2001 From: Tom Marshall Date: Thu, 2 Jun 2016 10:35:32 -0700 Subject: MTP: Fix crash when no storages are available Change-Id: I6d7202ade46a5d781a3db5a1a3bdde17c8e70a60 --- media/mtp/MtpServer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'media/mtp/MtpServer.cpp') diff --git a/media/mtp/MtpServer.cpp b/media/mtp/MtpServer.cpp index 3c91ffd..96ce81b 100644 --- a/media/mtp/MtpServer.cpp +++ b/media/mtp/MtpServer.cpp @@ -138,7 +138,7 @@ void MtpServer::removeStorage(MtpStorage* storage) { MtpStorage* MtpServer::getStorage(MtpStorageID id) { if (id == 0) - return mStorages[0]; + return mStorages.empty() ? NULL : mStorages[0]; for (size_t i = 0; i < mStorages.size(); i++) { MtpStorage* storage = mStorages[i]; if (storage->getStorageID() == id) -- cgit v1.1 From b3fbce13d6a0c30012e069c4a2131407beb31ab7 Mon Sep 17 00:00:00 2001 From: Tom Marshall Date: Mon, 6 Jun 2016 10:09:50 -0700 Subject: MtpServer: Fix concurrent access to mStorages * getStorage is called both internally with mMutex held and externally without mMutex held. Create getStorageLocked for internal use and make getStorage a wrapper. * hasStorage is only called internally with mMutex held. Make it a private method. Change-Id: I8f73310ad6cca14cd88b8e29f20cc181b3a4fac3 --- media/mtp/MtpServer.cpp | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-) (limited to 'media/mtp/MtpServer.cpp') diff --git a/media/mtp/MtpServer.cpp b/media/mtp/MtpServer.cpp index 96ce81b..d5f8ff4 100644 --- a/media/mtp/MtpServer.cpp +++ b/media/mtp/MtpServer.cpp @@ -137,20 +137,9 @@ void MtpServer::removeStorage(MtpStorage* storage) { } MtpStorage* MtpServer::getStorage(MtpStorageID id) { - if (id == 0) - return mStorages.empty() ? NULL : mStorages[0]; - for (size_t i = 0; i < mStorages.size(); i++) { - MtpStorage* storage = mStorages[i]; - if (storage->getStorageID() == id) - return storage; - } - return NULL; -} + Mutex::Autolock autoLock(mMutex); -bool MtpServer::hasStorage(MtpStorageID id) { - if (id == 0 || id == 0xFFFFFFFF) - return mStorages.size() > 0; - return (getStorage(id) != NULL); + return getStorageLocked(id); } void MtpServer::run() { @@ -260,6 +249,23 @@ void MtpServer::sendObjectUpdated(MtpObjectHandle handle) { sendEvent(MTP_EVENT_OBJECT_PROP_CHANGED, handle); } +MtpStorage* MtpServer::getStorageLocked(MtpStorageID id) { + if (id == 0) + return mStorages.empty() ? NULL : mStorages[0]; + for (size_t i = 0; i < mStorages.size(); i++) { + MtpStorage* storage = mStorages[i]; + if (storage->getStorageID() == id) + return storage; + } + return NULL; +} + +bool MtpServer::hasStorage(MtpStorageID id) { + if (id == 0 || id == 0xFFFFFFFF) + return mStorages.size() > 0; + return (getStorageLocked(id) != NULL); +} + void MtpServer::sendStoreAdded(MtpStorageID id) { ALOGV("sendStoreAdded %08X\n", id); sendEvent(MTP_EVENT_STORE_ADDED, id); @@ -543,7 +549,7 @@ MtpResponseCode MtpServer::doGetStorageInfo() { return MTP_RESPONSE_INVALID_PARAMETER; MtpStorageID id = mRequest.getParameter(1); - MtpStorage* storage = getStorage(id); + MtpStorage* storage = getStorageLocked(id); if (!storage) return MTP_RESPONSE_INVALID_STORAGE_ID; @@ -894,7 +900,7 @@ MtpResponseCode MtpServer::doSendObjectInfo() { if (mRequest.getParameterCount() < 2) return MTP_RESPONSE_INVALID_PARAMETER; MtpStorageID storageID = mRequest.getParameter(1); - MtpStorage* storage = getStorage(storageID); + MtpStorage* storage = getStorageLocked(storageID); MtpObjectHandle parent = mRequest.getParameter(2); if (!storage) return MTP_RESPONSE_INVALID_STORAGE_ID; -- cgit v1.1