[Merge] lp:~phablet-team/mtp/aosp-update into lp:mtp
Konrad Zapałowicz
konrad.zapalowicz at canonical.com
Tue Apr 12 13:25:58 UTC 2016
Review: Needs Fixing code
Minor issues mostly related to type-safety
Diff comments:
>
> === modified file 'include/MtpRequestPacket.h'
> --- include/MtpRequestPacket.h 2013-06-13 10:22:21 +0000
> +++ include/MtpRequestPacket.h 2016-04-12 12:54:31 +0000
> @@ -43,6 +43,10 @@
> inline MtpOperationCode getOperationCode() const { return getContainerCode(); }
> inline void setOperationCode(MtpOperationCode code)
> { return setContainerCode(code); }
> + inline int getParameterCount() const { return mParameterCount; }
> +
> +private:
> + int mParameterCount;
Can it be less than zero? If so then it is ok, otherwise it should be unsigned
> };
>
> }; // namespace android
>
> === modified file 'include/MtpStringBuffer.h'
> --- include/MtpStringBuffer.h 2013-06-13 10:22:21 +0000
> +++ include/MtpStringBuffer.h 2016-04-12 12:54:31 +0000
> @@ -19,6 +19,9 @@
>
> #include <stdint.h>
>
> +// Max Character number of a MTP String
> +#define MTP_STRING_MAX_CHARACTER_NUMBER 255
constexpr
> +
> namespace android {
>
> class MtpDataPacket;
>
> === modified file 'server/server.cpp'
> --- server/server.cpp 2015-02-13 15:19:49 +0000
> +++ server/server.cpp 2016-04-12 12:54:31 +0000
> @@ -148,13 +149,14 @@
> {
> static int storageID = MTP_STORAGE_REMOVABLE_RAM;
>
> + /* TODO check removable file system type to set maximum file size */
> MtpStorage *removable = new MtpStorage(
> - storageID,
> + storageID,
> path,
> name,
> 1024 * 1024 * 100, /* 100 MB reserved space, to avoid filling the disk */
> true,
> - 1024 * 1024 * 1024 * 2 /* 2GB arbitrary max file size */);
> + UINT64_C(4294967295) /* 4GB-1, we assume vfat here */);
I would prefer the magic number to be constexpr
>
> storageID++;
>
>
> === modified file 'src/MtpDataPacket.cpp'
> --- src/MtpDataPacket.cpp 2014-03-27 15:45:30 +0000
> +++ src/MtpDataPacket.cpp 2016-04-12 12:54:31 +0000
> @@ -53,104 +53,178 @@
> MtpPacket::putUInt32(MTP_CONTAINER_TRANSACTION_ID_OFFSET, id);
> }
>
> -uint16_t MtpDataPacket::getUInt16() {
> - int offset = mOffset;
> - uint16_t result = (uint16_t)mBuffer[offset] | ((uint16_t)mBuffer[offset + 1] << 8);
> - mOffset += 2;
> - return result;
> -}
> -
> -uint32_t MtpDataPacket::getUInt32() {
> - int offset = mOffset;
> - uint32_t result = (uint32_t)mBuffer[offset] | ((uint32_t)mBuffer[offset + 1] << 8) |
> +bool MtpDataPacket::getUInt8(uint8_t& value) {
> + if (mPacketSize - mOffset < sizeof(value))
> + return false;
> + value = mBuffer[mOffset++];
> + return true;
> +}
> +
> +bool MtpDataPacket::getUInt16(uint16_t& value) {
> + if (mPacketSize - mOffset < sizeof(value))
> + return false;
> + int offset = mOffset;
mOffset is size_t
> + value = (uint16_t)mBuffer[offset] | ((uint16_t)mBuffer[offset + 1] << 8);
> + mOffset += sizeof(value);
> + return true;
> +}
> +
> +bool MtpDataPacket::getUInt32(uint32_t& value) {
> + if (mPacketSize - mOffset < sizeof(value))
> + return false;
> + int offset = mOffset;
mOffset is size_t
> + value = (uint32_t)mBuffer[offset] | ((uint32_t)mBuffer[offset + 1] << 8) |
> ((uint32_t)mBuffer[offset + 2] << 16) | ((uint32_t)mBuffer[offset + 3] << 24);
> - mOffset += 4;
> - return result;
> + mOffset += sizeof(value);
> + return true;
> }
>
> -uint64_t MtpDataPacket::getUInt64() {
> +bool MtpDataPacket::getUInt64(uint64_t& value) {
> + if (mPacketSize - mOffset < sizeof(value))
> + return false;
> int offset = mOffset;
> - uint64_t result = (uint64_t)mBuffer[offset] | ((uint64_t)mBuffer[offset + 1] << 8) |
> + value = (uint64_t)mBuffer[offset] | ((uint64_t)mBuffer[offset + 1] << 8) |
> ((uint64_t)mBuffer[offset + 2] << 16) | ((uint64_t)mBuffer[offset + 3] << 24) |
> ((uint64_t)mBuffer[offset + 4] << 32) | ((uint64_t)mBuffer[offset + 5] << 40) |
> ((uint64_t)mBuffer[offset + 6] << 48) | ((uint64_t)mBuffer[offset + 7] << 56);
> - mOffset += 8;
> - return result;
> -}
> -
> -void MtpDataPacket::getUInt128(uint128_t& value) {
> - value[0] = getUInt32();
> - value[1] = getUInt32();
> - value[2] = getUInt32();
> - value[3] = getUInt32();
> -}
> -
> -void MtpDataPacket::getString(MtpStringBuffer& string)
> + mOffset += sizeof(value);
> + return true;
> +}
> +
> +bool MtpDataPacket::getUInt128(uint128_t& value) {
> + return getUInt32(value[0]) && getUInt32(value[1]) && getUInt32(value[2]) && getUInt32(value[3]);
> +}
> +
> +bool MtpDataPacket::getString(MtpStringBuffer& string)
> {
> - string.readFromPacket(this);
> + return string.readFromPacket(this);
> }
>
> Int8List* MtpDataPacket::getAInt8() {
> + uint32_t count;
> + if (!getUInt32(count))
> + return NULL;
> Int8List* result = new Int8List;
> - int count = getUInt32();
> - for (int i = 0; i < count; i++)
> - result->push_back(getInt8());
> + for (uint32_t i = 0; i < count; i++) {
> + int8_t value;
is allocated at every spin. could it be allocated before the loop and reused?
> + if (!getInt8(value)) {
> + delete result;
> + return NULL;
> + }
> + result->push_back(value);
> + }
> return result;
> }
>
> UInt8List* MtpDataPacket::getAUInt8() {
> + uint32_t count;
> + if (!getUInt32(count))
> + return NULL;
> UInt8List* result = new UInt8List;
> - int count = getUInt32();
> - for (int i = 0; i < count; i++)
> - result->push_back(getUInt8());
> + for (uint32_t i = 0; i < count; i++) {
> + uint8_t value;
is allocated at every spin. could it be allocated before the loop and reused?
> + if (!getUInt8(value)) {
> + delete result;
> + return NULL;
> + }
> + result->push_back(value);
> + }
> return result;
> }
>
> Int16List* MtpDataPacket::getAInt16() {
> + uint32_t count;
> + if (!getUInt32(count))
> + return NULL;
> Int16List* result = new Int16List;
> - int count = getUInt32();
> - for (int i = 0; i < count; i++)
> - result->push_back(getInt16());
> + for (uint32_t i = 0; i < count; i++) {
> + int16_t value;
is allocated at every spin. could it be allocated before the loop and reused?
> + if (!getInt16(value)) {
> + delete result;
> + return NULL;
> + }
> + result->push_back(value);
> + }
> return result;
> }
>
> UInt16List* MtpDataPacket::getAUInt16() {
> + uint32_t count;
> + if (!getUInt32(count))
> + return NULL;
> UInt16List* result = new UInt16List;
> - int count = getUInt32();
> - for (int i = 0; i < count; i++)
> - result->push_back(getUInt16());
> + for (uint32_t i = 0; i < count; i++) {
> + uint16_t value;
is allocated at every spin. could it be allocated before the loop and reused?
> + if (!getUInt16(value)) {
> + delete result;
> + return NULL;
> + }
> + result->push_back(value);
> + }
> return result;
> }
>
> Int32List* MtpDataPacket::getAInt32() {
> + uint32_t count;
> + if (!getUInt32(count))
> + return NULL;
> Int32List* result = new Int32List;
> - int count = getUInt32();
> - for (int i = 0; i < count; i++)
> - result->push_back(getInt32());
> + for (uint32_t i = 0; i < count; i++) {
> + int32_t value;
is allocated at every spin. could it be allocated before the loop and reused?
> + if (!getInt32(value)) {
> + delete result;
> + return NULL;
> + }
> + result->push_back(value);
> + }
> return result;
> }
>
> UInt32List* MtpDataPacket::getAUInt32() {
> + uint32_t count;
> + if (!getUInt32(count))
> + return NULL;
> UInt32List* result = new UInt32List;
> - int count = getUInt32();
> - for (int i = 0; i < count; i++)
> - result->push_back(getUInt32());
> + for (uint32_t i = 0; i < count; i++) {
> + uint32_t value;
> + if (!getUInt32(value)) {
> + delete result;
> + return NULL;
> + }
> + result->push_back(value);
> + }
> return result;
> }
>
> Int64List* MtpDataPacket::getAInt64() {
> + uint32_t count;
> + if (!getUInt32(count))
> + return NULL;
> Int64List* result = new Int64List;
> - int count = getUInt32();
> - for (int i = 0; i < count; i++)
> - result->push_back(getInt64());
> + for (uint32_t i = 0; i < count; i++) {
> + int64_t value;
> + if (!getInt64(value)) {
> + delete result;
> + return NULL;
> + }
> + result->push_back(value);
> + }
> return result;
> }
>
> UInt64List* MtpDataPacket::getAUInt64() {
> + uint32_t count;
> + if (!getUInt32(count))
> + return NULL;
> UInt64List* result = new UInt64List;
> - int count = getUInt32();
> - for (int i = 0; i < count; i++)
> - result->push_back(getUInt64());
> + for (uint32_t i = 0; i < count; i++) {
> + uint64_t value;
> + if (!getUInt64(value)) {
> + delete result;
> + return NULL;
> + }
> + result->push_back(value);
> + }
> return result;
> }
>
> @@ -333,7 +407,7 @@
>
> void MtpDataPacket::putString(const uint16_t* string) {
> int count = 0;
should be positive
> - for (int i = 0; i < 256; i++) {
> + for (int i = 0; i <= MTP_STRING_MAX_CHARACTER_NUMBER; i++) {
i is always positive
> if (string[i])
> count++;
> else
>
> === modified file 'src/MtpProperty.cpp'
> --- src/MtpProperty.cpp 2014-03-27 15:45:30 +0000
> +++ src/MtpProperty.cpp 2016-04-12 12:54:31 +0000
> @@ -146,28 +149,36 @@
> case MTP_TYPE_AINT128:
> case MTP_TYPE_AUINT128:
> mDefaultArrayValues = readArrayValues(packet, mDefaultArrayLength);
> - if (deviceProp)
> + if (!mDefaultArrayValues) return false;
> + if (deviceProp) {
> mCurrentArrayValues = readArrayValues(packet, mCurrentArrayLength);
> + if (!mCurrentArrayValues) return false;
> + }
> break;
> default:
> - readValue(packet, mDefaultValue);
> - if (deviceProp)
> - readValue(packet, mCurrentValue);
> - }
> - if (!deviceProp)
> - mGroupCode = packet.getUInt32();
> - mFormFlag = packet.getUInt8();
> + if (!readValue(packet, mDefaultValue)) return false;
> + if (deviceProp) {
> + if (!readValue(packet, mCurrentValue)) return false;
> + }
> + }
> + if (!deviceProp) {
> + if (!packet.getUInt32(mGroupCode)) return false;
> + }
> + if (!packet.getUInt8(mFormFlag)) return false;
>
> if (mFormFlag == kFormRange) {
> - readValue(packet, mMinimumValue);
> - readValue(packet, mMaximumValue);
> - readValue(packet, mStepSize);
> + if (!readValue(packet, mMinimumValue)) return false;
> + if (!readValue(packet, mMaximumValue)) return false;
> + if (!readValue(packet, mStepSize)) return false;
> } else if (mFormFlag == kFormEnum) {
> - mEnumLength = packet.getUInt16();
> + if (!packet.getUInt16(mEnumLength)) return false;
> mEnumValues = new MtpPropertyValue[mEnumLength];
> - for (int i = 0; i < mEnumLength; i++)
> - readValue(packet, mEnumValues[i]);
> + for (int i = 0; i < mEnumLength; i++) {
is is always positive
> + if (!readValue(packet, mEnumValues[i])) return false;
> + }
> }
> +
> + return true;
> }
>
> void MtpProperty::write(MtpDataPacket& packet) {
> @@ -535,19 +547,28 @@
> }
> }
>
> -MtpPropertyValue* MtpProperty::readArrayValues(MtpDataPacket& packet, int& length) {
> - length = packet.getUInt32();
> - if (length == 0)
> +MtpPropertyValue* MtpProperty::readArrayValues(MtpDataPacket& packet, uint32_t& length) {
wonder if the packet could be const
> + if (!packet.getUInt32(length)) return NULL;
> +
> + // Fail if resulting array is over 2GB. This is because the maximum array
> + // size may be less than SIZE_MAX on some platforms.
> + if (length == 0 ||
> + length >= INT32_MAX / sizeof(MtpPropertyValue)) {
> + length = 0;
> return NULL;
> + }
> MtpPropertyValue* result = new MtpPropertyValue[length];
> - for (int i = 0; i < length; i++)
> - readValue(packet, result[i]);
> + for (uint32_t i = 0; i < length; i++)
> + if (!readValue(packet, result[i])) {
> + delete result;
> + return NULL;
> + }
> return result;
> }
>
> -void MtpProperty::writeArrayValues(MtpDataPacket& packet, MtpPropertyValue* values, int length) {
> +void MtpProperty::writeArrayValues(MtpDataPacket& packet, MtpPropertyValue* values, uint32_t length) {
> packet.putUInt32(length);
> - for (int i = 0; i < length; i++)
> + for (uint32_t i = 0; i < length; i++)
> writeValue(packet, values[i]);
> }
>
>
> === modified file 'src/MtpRequestPacket.cpp'
> --- src/MtpRequestPacket.cpp 2013-06-13 10:22:21 +0000
> +++ src/MtpRequestPacket.cpp 2016-04-12 12:54:31 +0000
> @@ -24,11 +24,13 @@
> #include "MtpRequestPacket.h"
>
> #include <usbhost/usbhost.h>
> +#include <glog/logging.h>
>
> namespace android {
>
> MtpRequestPacket::MtpRequestPacket()
> - : MtpPacket(512)
> + : MtpPacket(512),
magic number
> + mParameterCount(0)
> {
> }
>
--
https://code.launchpad.net/~phablet-team/mtp/aosp-update/+merge/291626
Your team Ubuntu Phablet Team is subscribed to branch lp:mtp.
More information about the Ubuntu-reviews
mailing list