mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.36k stars 1.33k forks source link

Crash calling -expressionValueWithObject:context: on vararg expression functions #11541

Open 1ec5 opened 6 years ago

1ec5 commented 6 years ago

When initializing an NSExpression, passing four or more arguments to a custom vararg function causes a crash due to an uncaught exception in NSExpression’s implementation. This crash reproduces when using mgl_match: (#11464), mgl_case: (#11278), or any of the aftermarket control flow functions being added in #11472. For example:

NSExpression *expression = [NSExpression expressionWithFormat:@"FUNCTION(num = 1, 'mgl_case:', 1, num = 2, 2, 3)"]
[expression expressionValueWithObject:nil context:nil];
-[MGLExpressionTests testGenericExpressionObject] : failed: caught "NSInvalidArgumentException", "-[NSInvocation setArgument:atIndex:]: index (3) out of bounds [-1, 2]" ``` /Users/mxn/hub/mapbox-gl-native-3/platform/darwin/test/MGLExpressionTests.mm:676: error: -[MGLExpressionTests testGenericExpressionObject] : failed: caught "NSInvalidArgumentException", "-[NSInvocation setArgument:atIndex:]: index (3) out of bounds [-1, 2]" ( 0 CoreFoundation 0x00007fff459d6fcb __exceptionPreprocess + 171 1 libobjc.A.dylib 0x00007fff6c678c76 objc_exception_throw + 48 2 CoreFoundation 0x00007fff4594e0a4 -[NSInvocation setArgument:atIndex:] + 452 3 Foundation 0x00007fff47a40140 -[NSFunctionExpression expressionValueWithObject:context:] + 1147 4 test 0x00000001074ce0cf -[MGLExpressionTests testGenericExpressionObject] + 3599 5 CoreFoundation 0x00007fff4594e9dc __invoking___ + 140 6 CoreFoundation 0x00007fff4594e8b0 -[NSInvocation invoke] + 320 7 XCTest 0x0000000100361b36 __24-[XCTestCase invokeTest]_block_invoke.272 + 50 8 XCTest 0x00000001003b1416 -[XCTMemoryChecker _assertInvalidObjectsDeallocatedAfterScope:] + 37 9 XCTest 0x00000001003618cc __24-[XCTestCase invokeTest]_block_invoke + 722 10 XCTest 0x00000001003aa897 -[XCUITestContext performInScope:] + 183 11 XCTest 0x00000001003615ef -[XCTestCase invokeTest] + 141 12 XCTest 0x000000010036272d __26-[XCTestCase performTest:]_block_invoke.379 + 42 13 XCTest 0x00000001003ae246 +[XCTContext runInContextForTestCase:block:] + 163 14 XCTest 0x00000001003620c9 -[XCTestCase performTest:] + 608 15 XCTest 0x000000010035df8e __27-[XCTestSuite performTest:]_block_invoke + 363 16 XCTest 0x000000010035d8f5 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 26 17 XCTest 0x000000010035daf2 -[XCTestSuite performTest:] + 239 18 XCTest 0x000000010035df8e __27-[XCTestSuite performTest:]_block_invoke + 363 19 XCTest 0x000000010035d8f5 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 26 20 XCTest 0x000000010035daf2 -[XCTestSuite performTest:] + 239 21 XCTest 0x000000010035df8e __27-[XCTestSuite performTest:]_block_invoke + 363 22 XCTest 0x000000010035d8f5 -[XCTestSuite _performProtectedSectionForTest:testSection:] + 26 23 XCTest 0x000000010035daf2 -[XCTestSuite performTest:] + 239 24 XCTest 0x00000001003bd0e2 __44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke + 40 25 XCTest 0x0000000100378dca -[XCTestObservationCenter _observeTestExecutionForBlock:] + 477 26 XCTest 0x00000001003bcf80 -[XCTTestRunSession runTestsAndReturnError:] + 281 27 XCTest 0x000000010034daf9 -[XCTestDriver runTestsAndReturnError:] + 314 28 XCTest 0x00000001003ad586 _XCTestMain + 833 29 xctest 0x00000001000023d2 xctest + 9170 30 libdyld.dylib 0x00007fff6d268115 start + 1 ) ```

Apparently NSExpression assumes a fixed size for varargs when building an NSInvocation. [This Stack Overflow post](https://stackoverflow.com/a/18116108/4585461) suggests that we should pair any vararg method with a `va_list` method. Perhaps NSExpression somehow knows to look for that method when building the invocation; not sure. This is a mostly academic point. The conversion to JSON objects in `+[NSExpression mgl_expressionWithJSONObject:]` works just fine, and underlying Objective-C implementations of these vararg methods all raise a “not implemented” exception anyways. But it would be a better user experience to see that exception than the one about out-of-bounds access. /cc @fabian-guerra @anandthakker
stale[bot] commented 5 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] commented 5 years ago

This issue has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.