[Feature] Adding Appearance support (part of advertisement data)

[MrCsabaToth]

Fixes #766 and Fixes #202

[chipweinberger]

looks good

[MrCsabaToth]

I verified on Android 12 that it works. Since the code is the same it should be fine for the earlier test platforms as well (Android 13, 9, 6)

[chipweinberger]

actually, I dont think this code is correct.

You are just parsing the bytes until you find 0x19 and then assume it is the appearance value.

but 0x19 can appear in a service uuid or a msd, among others. It will get confused and return the wrong data.

I'll need to undo this change.

[chipweinberger]

you would need to handle all dataTypes.

this is a pretty complicated code change. You need to fully parse the advertising bytes yourself.

last, this code needs bounds checks

return scanRecordBytes[byteIndex + 2] * 256 + scanRecordBytes[byteIndex + 1];

[MrCsabaToth]

There's no reason to parse the data structure fully. Since this is only about appearance I only need to fish for the 0x19 dataType. I can simply stride over all other types, because I don't care about their content, I'm not interested in them. This is also the reason why I can simply return once I find my target. It'd be only needed to be aware of other data type if we'd extract anything this way. However all other data has their on calls, they are handled in a different way.

[MrCsabaToth]

Can we chat somewhere?

[MrCsabaToth]

The code is correct, tested with many devices around me.

[chipweinberger]

Okay, you're mostly right. We don't need to handle all dataTypes. The length of each dataType is included in the advertising data, allowing us to easily skip them.

The complicated part of your code is that you are increasing byteIndex twice.

                int length = scanRecordBytes[byteIndex++];
...
                byteIndex += length;

Imo the code should be more like this, where we only jump forward based on the dataType length.

This code also has a bounds check in the beginning, preventing a malformed advertisement from crashing user apps, and also is a security risk.

void parse_advertisement(const uint8_t *packet, size_t length) {
    size_t index = 0;

    while (index < length) {
        uint8_t field_length = packet[index];
        if (field_length == 0 || index + field_length >= length) {
            break; // Invalid field length or end of packet
        }

        if (index + 2 >= length) {
           break; // bounds check
        }

        uint8_t field_type = packet[index + 1];
        const uint8_t *field_data = &packet[index + 2];
        size_t field_data_length = field_length - 1;

        switch (field_type) {
            case ADV_TYPE_FLAGS:
                // Handle Flags
                printf("Flags: %02X\n", field_data[0]);
                break;
            case ADV_TYPE_SHORT_LOCAL_NAME:
            case ADV_TYPE_COMPLETE_LOCAL_NAME:
                printf("Local Name: %.*s\n", (int)field_data_length, field_data);
                break;
            case ADV_TYPE_MANUFACTURER_SPECIFIC_DATA:
                print_manufacturer_data(field_data, field_data_length);
                break;
            default:
                printf("Unknown Field: Type=%02X, Length=%zu\n", field_type, field_data_length);
                break;
        }

        index += field_length + 1;
    }
}

[MrCsabaToth]

Some concern might be performance: since this be a for iteration cycle per ScanRecord (under the hood of getAdvertosimgDataMap - which gathers all data type payload chunks organized into a Map - on Android 13+, or a simplified loop by me for pre Android 13). I think the other native functions which essentially cater data found in this same advertisement byte array also parses it in a targeted way or maybe look into a cached version.

The amount of bytes is not much IMHO (usually in the ballpark of some dozen bytes), and I think the more performance sensitive is the data communication itself once someone connects to a device. The byte parsing is already happening.

[MrCsabaToth]

You are right, I can make code change regarding extra bound check and eliminating ++ operators with +=. Would that be sufficient?

[chipweinberger]

something like this is what im thinking

    int getAppearanceFromScanRecord(ScanRecord adv) {

        if (Build.VERSION.SDK_INT >= 33) { // Android 13

            Map<Integer, byte[]> map = adv.getAdvertisingDataMap();
            if (map.containsKey(ScanRecord.DATA_TYPE_APPEARANCE)) {
                byte[] bytes = map.get(ScanRecord.DATA_TYPE_APPEARANCE);
                if (bytes.length == 2) {
                   return bytes[1] * 256 + bytes[0];
                }
            } 

            return 0;
        } 

        // For API Level 21+
        byte[] bytes = adv.getBytes();

        int n = 0;

        while (n < bytes.length) {

            int fieldLen = bytes[n];

            // no more data
            if (fieldLen == 0) {
                break; 
            }

            // end of packet
            if(fieldLen + n + 1 >= bytes.length) {
                break; 
            }

            int dataType = bytes[n + 1];

            // no more data
            if (dataType == 0) {
                break;
            }

            // appearance magic byte
            if (dataType == 0x19 && fieldLen == 2) { 
                return bytes[n + 3] * 256 + bytes[n + 2];
            }

            n += fieldLen + 1;
        }

        return 0;
    }

[MrCsabaToth]

I'll use that, adding a null check for adv as well.

[chipweinberger]

I'd add the null check before getAppearanceFromScanRecord is called, like we do for the other adv functions.

[MrCsabaToth]

I'm running some dry runs, seems like that if(fieldLen + n + 1 >= bytes.length) { might be off-by-one too restrictive.

Examples:

1. [7 -1 76 0 18 2 0 1] 7 length data, 8 bytes. I know this is not appearance, but when this byte array is checked it's ignored, because 7 + 0 + 1 >= 8.
2. [2 1 26 13 -1 76 0 22 8 0 109 -23 -84 23 -33 20 16], broken up to data chunks: [2 1 26] [13 -1 76 0 22 8 0 109 -23 -84 23 -33 20 16] for the second chunk 13 + 3 + 1 >= 17
3. [2 1 26 2 10 12 11 -1 76 0 16 6 10 26 -118 43 -90 -124], broken up to data chunks: [2 1 28] [2 10 12] [11 -1 76 0 16 6 10 26 -118 43 -90 -124], for the third chunk 11 + 6 + 1 >= 18

So it seems it always disses the last data chunk, so it the appearance would be mentioned last it wouldn't be picked up. I'm thinking it should be if (fieldLen + n > bytes.length - 1) {?

[chipweinberger]

no i think

fieldLen + n + 1 > bytes.length - 1

lets say fieldlen = 2 (2 appearance bytes) length = 4 (lenByte, dataTypeByte, appearanceByte0, appearanceByte1) n = 0

2+0+1 > 4-1 3>3 (false)

if length = 3, the check would fail, which is what we want, because we need at least 4 bytes for appearance.

[chipweinberger]

ah, acoording to chatGPT, fieldlen includes the dataType byte.

so ya, should be fieldLen + n > bytes.length - 1

&

if (dataType == 0x19 && fieldLen == 3) {

[MrCsabaToth]

After some tests I also discovered one (hopefully) last part which could be made more robust: since the byte type in Java is signed, a malformed byte array can cause int fieldLen = bytes[n]; to result in a negative fieldLen and that later this would cause a negative n with n += fieldLen + 1; at the end of the loop, and then that would crash the loop. We need to make sure the fieldLen is not negative. Maybe int fieldLen = Math.max(bytes[n], 0);, or maybe a separate extra if check?

Also, I'm not particular if we'll use fieldLen + n > bytes.length - 1 style check, in which case the right side represents the 0-based index into the array, whereas fieldLen + n + 1 > bytes.length would compare straight up length on the right side.

[chipweinberger]

I cannot be negative because it comes from a byte array.

99% sure

[MrCsabaToth]

I think Java byte type is signed ("8-bit signed (+ ive or - ive) values from -128 to 127."). The values I was intentionally mangling / fuzzing in the test came from my earlier debug logs from real world data, such as 3 25 65 3 2 1 6 3 3 -79 -6 9 -1 0 0 -31 -123 72 -97 -27 -68 3 9 77 82 30 -1 1 1 1 0 0 0 0 0 0 0 43 -120 0 0 22 1 0 100 2 0 0 0 0 -81 27 -34 0 0 0 (WAHOO TICKR Heart Rate Band), notice the negative values, maybe only the print makes them that? Bard also thinks it a signed value, but I don't state it with 100% certainty.

[MrCsabaToth]

Or simply just if (fieldLen <= 0) {

[MrCsabaToth]

Dart converted (very minimal conversion needed) ran on dartpad.dev:

int getAppearanceFromScanRecord(List<int> bytes, int offByOne) {
  int n = 0;
  while (n < bytes.length) {
    int fieldLen = bytes[n];

    // no more or malformed data
    if (fieldLen <= 0) {
      break; 
    }

    // end of packet
    if (fieldLen + n + offByOne >= bytes.length) {
      break; 
    }

    int dataType = bytes[n + 1];

    // no more data
    if (dataType == 0) {
      break;
    }

    // appearance magic byte
    if (dataType == 0x19 && fieldLen == 3) { 
      return bytes[n + 3] * 256 + bytes[n + 2];
    }

    n += fieldLen + 1;
  }

  return 0;
}

void main() {
  print(getAppearanceFromScanRecord([3, 25, 65, 3, 2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 1));
  print(getAppearanceFromScanRecord([3, 25, 65, 3, 2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 0));

  print(getAppearanceFromScanRecord([2, 1, 6, 3, 25, 65, 3, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 1));
  print(getAppearanceFromScanRecord([2, 1, 6, 3, 25, 65, 3, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 0));

  print(getAppearanceFromScanRecord([2, 1, 6, 3, 3, -79, -6, 3, 25, 65, 3, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 1));
  print(getAppearanceFromScanRecord([2, 1, 6, 3, 3, -79, -6, 3, 25, 65, 3, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 0));

  print(getAppearanceFromScanRecord([2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 25, 65, 3, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 1));
  print(getAppearanceFromScanRecord([2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 25, 65, 3, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 0));

  print(getAppearanceFromScanRecord([2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 3, 25, 65, 3, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 1));
  print(getAppearanceFromScanRecord([2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 3, 25, 65, 3, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 0));

  print(getAppearanceFromScanRecord([2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0, 3, 25, 65, 3], 1));
  print(getAppearanceFromScanRecord([2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0, 3, 25, 65, 3], 0));

  // Mis-offsetted
  print(getAppearanceFromScanRecord([3, 2, 1, 3, 25, 65, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 1));
  print(getAppearanceFromScanRecord([3, 2, 1, 3, 25, 65, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 0));
  // Mis-offsetted
  print(getAppearanceFromScanRecord([3, 2, 1, 6, 3, 3, -79, -6, 9, -1, 3, 25, 65, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 1));
  print(getAppearanceFromScanRecord([3, 2, 1, 6, 3, 3, -79, -6, 9, -1, 3, 25, 65, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0], 0));
  // Mis-offsetted
  print(getAppearanceFromScanRecord([3, 2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0, 3, 25, 65], 1));
  print(getAppearanceFromScanRecord([3, 2, 1, 6, 3, 3, -79, -6, 9, -1, 0, 0, -31, -123, 72, -97, -27, -68, 3, 9, 77, 82, 30, -1, 1, 1, 1, 0, 0, 0, 0, 0, 0, 0, 43, -120, 0, 0, 22, 1, 0, 100, 2, 0, 0, 0, 0, -81, 27, -34, 0, 0, 0, 3, 25, 65], 0));
}

[chipweinberger]

https://github.com/pauldemarco/flutter_blue/blob/master/android/src/main/java/com/pauldemarco/flutter_blue/AdvertisementParser.java

[MrCsabaToth]

https://stackoverflow.com/questions/3108297/james-goslings-explanation-of-why-javas-byte-is-signed

[chipweinberger]

you should probably convert it to unsigned, 0-255

[chipweinberger]

i think this code does that

int length = data.get() & 0xFF;

[MrCsabaToth]

No need to convert. The maximum length of the advertisement array is bellow 128 (I think it's 31+31 bytes), so if it's "negative" we can just simply deem the data tainted and stop processing. That's the one character change, the extra "<" character in if (fieldLen <= 0) {

1. Only one character change
2. No extra arithmetic
3. IMHO <= is more bullet proof defensive programming

However we can add & FF when we interpret the payload bytes.

[chipweinberger]

I think this is the complete code

int getAppearanceFromScanRecord(List<int> bytes) {
  int n = 0;
  while (n < bytes.length) {
    int fieldLen = bytes[n] & 0xFF;

    // no more or malformed data
    if (fieldLen <= 0) {
      break; 
    }

    // end of packet
    if (fieldLen + n >= bytes.length - 1) {
      break; 
    }

    int dataType = bytes[n + 1];

    // no more data
    if (dataType == 0) {
      break;
    }

    // appearance magic byte
    if (dataType == 0x19 && fieldLen == 3) { 
      return bytes[n + 3] * 256 + bytes[n + 2];
    }

    n += fieldLen + 1;
  }

  return 0;
}

[MrCsabaToth]

I'll modify my PR!

[MrCsabaToth]

The final code has a section for API level 33+. PR: https://github.com/boskokg/flutter_blue_plus/pull/791/