Fixes aggregate reporting cuts off bucket value
Bug: 269267931
Aggregate report bucket can hold a value up to 16 bytes. This value is
cbor encoded that holds a byte array. This byte array is
created/converted using the function BigInteger#toByteArray. BigInteger
sometimes uses 1 bit for the sign, therefore doing a
bigInteger.toByteArray().length can result in 1 more byte than expected
to hold such bit. This fix checks for the sign bit and if such sign is
present, that value won't be copied into the bucket.
Test: atest AdServicesServiceCoreUnitTests
Change-Id: Ia9e48c63258d3a3f617d71156d3ca2e0e0dc5494
diff --git a/adservices/service-core/java/com/android/adservices/service/measurement/aggregation/AggregateCryptoConverter.java b/adservices/service-core/java/com/android/adservices/service/measurement/aggregation/AggregateCryptoConverter.java
index 888799f..85e94d8 100644
--- a/adservices/service-core/java/com/android/adservices/service/measurement/aggregation/AggregateCryptoConverter.java
+++ b/adservices/service-core/java/com/android/adservices/service/measurement/aggregation/AggregateCryptoConverter.java
@@ -176,9 +176,12 @@
.array();
final byte[] bucket = new byte[AGGREGATE_HISTOGRAM_BUCKET_BYTE_SIZE];
final byte[] src = contribution.getKey().toByteArray();
- final int length = Math.min(src.length, AGGREGATE_HISTOGRAM_BUCKET_BYTE_SIZE);
+ final int bytesExcludingSign = (int) Math.ceil(contribution.getKey().bitLength() / 8d);
+ final int length = Math.min(bytesExcludingSign, AGGREGATE_HISTOGRAM_BUCKET_BYTE_SIZE);
final int position = bucket.length - length;
- System.arraycopy(src, /* srcPos = */ 0, bucket, position, length);
+ // Excluding sign bit that BigInteger#toByteArray adds to the first element of the array
+ final int srcPosExcludingSign = src[0] == 0 ? 1 : 0;
+ System.arraycopy(src, srcPosExcludingSign, bucket, position, length);
final Map dataMap = new Map();
dataMap.put(new UnicodeString("bucket"), new ByteString(bucket));
diff --git a/adservices/tests/unittest/service-core/assets/msmt_e2e_tests/aggregatable_large_keys.json b/adservices/tests/unittest/service-core/assets/msmt_e2e_tests/aggregatable_large_keys.json
index 8460bb1..f0cb8cb 100644
--- a/adservices/tests/unittest/service-core/assets/msmt_e2e_tests/aggregatable_large_keys.json
+++ b/adservices/tests/unittest/service-core/assets/msmt_e2e_tests/aggregatable_large_keys.json
@@ -113,7 +113,7 @@
"attribution_destination": "android-app://example.2d1.test",
"source_site": "android-app://example.1s1.test",
"histograms": [
- {"key": "0xfbec9fbde89afbfb7bdbafd97bcbfd", "value": 32768},
+ {"key": "0xfbec9fbde89afbfb7bdbafd97bcbfdf9", "value": 32768},
{"key": "0xa85", "value": 1664}
]
}
diff --git a/adservices/tests/unittest/service-core/src/com/android/adservices/service/measurement/E2EMockTest.java b/adservices/tests/unittest/service-core/src/com/android/adservices/service/measurement/E2EMockTest.java
index d9079ed..27302a3 100644
--- a/adservices/tests/unittest/service-core/src/com/android/adservices/service/measurement/E2EMockTest.java
+++ b/adservices/tests/unittest/service-core/src/com/android/adservices/service/measurement/E2EMockTest.java
@@ -485,6 +485,7 @@
Object value =
"0x"
+ new BigInteger(
+ 1,
((ByteString) m.get(new UnicodeString("bucket")))
.getBytes())
.toString(16);
@@ -494,6 +495,7 @@
.put(
AggregateHistogramKeys.VALUE,
new BigInteger(
+ 1,
((ByteString)
m.get(
new UnicodeString(
diff --git a/adservices/tests/unittest/service-core/src/com/android/adservices/service/measurement/aggregation/AggregateCryptoConverterTest.java b/adservices/tests/unittest/service-core/src/com/android/adservices/service/measurement/aggregation/AggregateCryptoConverterTest.java
index 4101f6b..eb4f0bc 100644
--- a/adservices/tests/unittest/service-core/src/com/android/adservices/service/measurement/aggregation/AggregateCryptoConverterTest.java
+++ b/adservices/tests/unittest/service-core/src/com/android/adservices/service/measurement/aggregation/AggregateCryptoConverterTest.java
@@ -319,6 +319,36 @@
}
@Test
+ public void testEncodeWithCbor_valuesAtUpperBoundLimit() throws Exception {
+ final List<AggregateHistogramContribution> contributions = new ArrayList<>();
+ final String bucketUpperBound = "340282366920938463463374607431768211455"; // 16 bytes
+ final int valueUpperBound = 15; // 4 bytes
+ final AggregateHistogramContribution contribution =
+ new AggregateHistogramContribution.Builder()
+ .setKey(new BigInteger(bucketUpperBound))
+ .setValue(valueUpperBound)
+ .build();
+ contributions.add(contribution);
+
+ final byte[] encoded = AggregateCryptoConverter.encodeWithCbor(contributions);
+ final List<DataItem> dataItems =
+ new CborDecoder(new ByteArrayInputStream(encoded)).decode();
+
+ final Map payload = (Map) dataItems.get(0);
+ final Array payloadArray = (Array) payload.get(new UnicodeString("data"));
+
+ assertEquals(1, payloadArray.getDataItems().size());
+ assertEquals("histogram", payload.get(new UnicodeString("operation")).toString());
+ assertTrue(
+ payloadArray.getDataItems().stream()
+ .anyMatch(
+ i ->
+ isFound((Map) i, "bucket", bucketUpperBound)
+ && isFound(
+ (Map) i, "value", "" + valueUpperBound)));
+ }
+
+ @Test
public void testEncryptWithHpke_successfully() {
byte[] encoded =
AggregateCryptoConverter.encryptWithHpke(
@@ -415,7 +445,9 @@
private boolean isFound(Map map, String name, String value) {
return new BigInteger(value)
- .equals(new BigInteger(((ByteString) map.get(new UnicodeString(name))).getBytes()));
+ .equals(
+ new BigInteger(
+ 1, ((ByteString) map.get(new UnicodeString(name))).getBytes()));
}
private int getBytesLength(Map map, String keyName) {
