Opened 3 years ago

Closed 3 years ago

#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 2n 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

Change History (4)

comment:1 Changed 3 years ago by asmirnov

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 3 years ago by sergz

  • Description modified (diff)

Added more details about it.

comment:3 Changed 3 years ago by abpbot

A commit referencing this issue has landed:
Issue 5239 - JsValue access crash

comment:4 Changed 3 years ago by asmirnov

  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.