Opened on 05/12/2017 at 11:52:01 AM

Closed on 05/12/2017 at 03:44:29 PM

#5239 closed defect (fixed)

JsValue access crash

Reported by:	asmirnov	Assignee:
Priority:	P2	Milestone:
Module:	Libadblockplus-Android	Keywords:
Cc:	sergz	Blocked By:
Blocking:		Platform:	Android
Ready:	yes	Confidential:	no
Tester:	Unknown	Verified working:	no
Review URL(s):	https://codereview.adblockplus.org/29437563/

Description (last modified by sergz)

Environment

In Java classes we keep a pointer to a native (C++) class in a Java long (64 bits) member, however, in order to construct that Java class we call JNIEnv::NewObject(jclass, jmethodId, ...) method which is essentially calling Java constructor accepting Java long. This method JNIEnv::NewObject takes a variable number of arguments and converts them to corresponding Java values, so it tries to read the value as 64 bit value but currently we are passing a 32 bit value (our current builds for ARM are 32bit, armeabi-v7a, therefore pointer size is 32 bits), so it reads some irrelevant value.

What to change

Before passing a pointer value to JNIEnv::NewObject it should be converted into corresponding 64 bit value jlong (aka long long).

In order to do it for 32 bit platform, we firstly need to say that it is unsigned integer (e.g. uintptr_t) and then convert it to 64 bit jlong (aka long long) because a 32 bit unsigned value can be represented in a 64 bit signed value (see https://isocpp.org/std/the-standard, e.g. from 4.7 Integral conversions of November 2014 working draft:

If the destination type is signed, the value is unchanged if it can be represented in the destination type; otherwise, the value is implementation-defined.

For 64 bit platform, according to the standard the value is implementation-defined and it seems the implementation with current compilers is fine, namely it leaves the bits unchanged, so we likely don't have to do anything with it, to have tests would be useful though (is someone willing to share the assembly code (for release of course), could be interesting to check it).

Conversion from signed to unsigned (jlong -> pointer) is defined

If the destination type is unsigned, the resulting value is the least unsigned integer congruent to the source integer (modulo 2ⁿ where n is the number of bits used to represent the unsigned type). [ Note: In a two’s complement representation, this conversion is conceptual and there is no change in the bit pattern (if there is no truncation). — end note ]

so, nothing to do with it.

How to reproduce

Run AdblockWebView app on hardware Android device (not emulator)
Wait for the crash

...

Observed behaviour

The app crashes

Expected behaviour

The app does not crash

Summary:

Description:

You may use WikiFormatting here.

=== Environment ===
In Java classes we keep a pointer to a native (C++) class in a Java `long` (64 bits) member, however, in order to construct that Java class we call `JNIEnv::NewObject(jclass, jmethodId, ...)` method which is essentially calling Java constructor accepting Java `long`. This method `JNIEnv::NewObject` takes a variable number of arguments and converts them to corresponding Java values, so it tries to read the value as 64 bit value but currently we are passing a 32 bit value (our current builds for ARM are 32bit, armeabi-v7a, therefore pointer size is 32 bits), so it reads some irrelevant value.

=== What to change ===
Before passing a pointer value to `JNIEnv::NewObject` it should be converted into corresponding 64 bit value `jlong` (aka `long long`).

In order to do it for 32 bit platform, we firstly need to say that it is unsigned integer (e.g. `uintptr_t`) and then convert it to 64 bit `jlong` (aka `long long`) because a 32 bit unsigned value can be represented in a 64 bit signed value (see https://isocpp.org/std/the-standard, e.g. from `4.7 Integral conversions` of ''November 2014 working draft'':
> If the destination type is signed, the value is unchanged if it can be represented in the destination type; otherwise, the value is implementation-defined.
).

For 64 bit platform, according to the standard the value is `implementation-defined` and it seems the implementation with current compilers is fine, namely it leaves the bits unchanged, so we likely don't have to do anything with it, to have tests would be useful though (is someone willing to share the assembly code (for release of course), could be interesting to check it).

Conversion from signed to unsigned (`jlong` -> pointer) is defined
> If the destination type is unsigned, the resulting value is the least unsigned integer congruent to the source integer (modulo 2^n^ where n is the number of bits used to represent the unsigned type). [ Note: In a two’s complement representation, this conversion is conceptual and there is no change in the bit pattern (if there is no truncation). — end note ]
so, nothing to do with it.

=== How to reproduce ===
 1. Run AdblockWebView app on hardware Android device (not emulator)
 2. Wait for the crash
...

=== Observed behaviour ===
The app crashes

=== Expected behaviour ===
The app does not crash

Type:

Priority:

Milestone:

Module:

Keywords:

Cc:

Blocked By:

Blocking:

Platform:

Ready:

Confidential:

Tester:

Verified working:

Review URL(s):

Context Navigation

#5239 closed defect (fixed)

JsValue access crash

Description (last modified by sergz)

Environment

What to change

How to reproduce

Observed behaviour

Expected behaviour

Attachments (0)

Change History (4)

comment:1 Changed on 05/12/2017 at 11:58:13 AM by asmirnov

comment:2 Changed on 05/12/2017 at 03:01:54 PM by sergz

comment:3 Changed on 05/12/2017 at 03:43:47 PM by abpbot

comment:4 Changed on 05/12/2017 at 03:44:29 PM by asmirnov

Add Comment

Modify Ticket

Changed by kzar

Download in other formats:

Summary:
Description:	You may use WikiFormatting here. === Environment === In Java classes we keep a pointer to a native (C++) class in a Java `long` (64 bits) member, however, in order to construct that Java class we call `JNIEnv::NewObject(jclass, jmethodId, ...)` method which is essentially calling Java constructor accepting Java `long`. This method `JNIEnv::NewObject` takes a variable number of arguments and converts them to corresponding Java values, so it tries to read the value as 64 bit value but currently we are passing a 32 bit value (our current builds for ARM are 32bit, armeabi-v7a, therefore pointer size is 32 bits), so it reads some irrelevant value. === What to change === Before passing a pointer value to `JNIEnv::NewObject` it should be converted into corresponding 64 bit value `jlong` (aka `long long`). In order to do it for 32 bit platform, we firstly need to say that it is unsigned integer (e.g. `uintptr_t`) and then convert it to 64 bit `jlong` (aka `long long`) because a 32 bit unsigned value can be represented in a 64 bit signed value (see https://isocpp.org/std/the-standard, e.g. from `4.7 Integral conversions` of ''November 2014 working draft'': > If the destination type is signed, the value is unchanged if it can be represented in the destination type; otherwise, the value is implementation-defined. ). For 64 bit platform, according to the standard the value is `implementation-defined` and it seems the implementation with current compilers is fine, namely it leaves the bits unchanged, so we likely don't have to do anything with it, to have tests would be useful though (is someone willing to share the assembly code (for release of course), could be interesting to check it). Conversion from signed to unsigned (`jlong` -> pointer) is defined > If the destination type is unsigned, the resulting value is the least unsigned integer congruent to the source integer (modulo 2^n^ where n is the number of bits used to represent the unsigned type). [ Note: In a two’s complement representation, this conversion is conceptual and there is no change in the bit pattern (if there is no truncation). — end note ] so, nothing to do with it. === How to reproduce === 1. Run AdblockWebView app on hardware Android device (not emulator) 2. Wait for the crash ... === Observed behaviour === The app crashes === Expected behaviour === The app does not crash
Type:		Priority:
Milestone:		Module:
Keywords:		Cc:
Blocked By:		Blocking:
Platform:		Ready:
Confidential:		Tester:
Verified working:
Review URL(s):	https://codereview.adblockplus.org/29437563/