Avoid undefined behavior: applying zero offset to null pointer

tamird · tamird · commit 770653e3ba67 · 2023-02-27T13:56:21.000-05:00
Before this change we see the following UBSAN stack trace in Fuchsia: #0 0x000021e4213b3302 in AcpiDsInitAmlWalk(ACPI_WALK_STATE*, ACPI_PARSE_OBJECT*, ACPI_NAMESPACE_NODE*, UINT8*, UINT32, ACPI_EVALUATE_INFO*, UINT8) ../../third_party/acpica/source/components/dispatcher/dswstate.c:682 <platform-bus-x86.so>+0x233302 #1.2 0x000020d0f660777f in ubsan_GetStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:41 <libclang_rt.asan.so>+0x3d77f #1.1 0x000020d0f660777f in MaybePrintStackTrace() compiler-rt/lib/ubsan/ubsan_diag.cpp:51 <libclang_rt.asan.so>+0x3d77f #1 0x000020d0f660777f in ~ScopedReport() compiler-rt/lib/ubsan/ubsan_diag.cpp:387 <libclang_rt.asan.so>+0x3d77f #2 0x000020d0f660b96d in handlePointerOverflowImpl() compiler-rt/lib/ubsan/ubsan_handlers.cpp:809 <libclang_rt.asan.so>+0x4196d #3 0x000020d0f660b50d in compiler-rt/lib/ubsan/ubsan_handlers.cpp:815 <libclang_rt.asan.so>+0x4150d #4 0x000021e4213b3302 in AcpiDsInitAmlWalk(ACPI_WALK_STATE*, ACPI_PARSE_OBJECT*, ACPI_NAMESPACE_NODE*, UINT8*, UINT32, ACPI_EVALUATE_INFO*, UINT8) ../../third_party/acpica/source/components/dispatcher/dswstate.c:682 <platform-bus-x86.so>+0x233302 #5 0x000021e4213e2369 in AcpiDsCallControlMethod(ACPI_THREAD_STATE*, ACPI_WALK_STATE*, ACPI_PARSE_OBJECT*) ../../third_party/acpica/source/components/dispatcher/dsmethod.c:605 <platform-bus-x86.so>+0x262369 #6 0x000021e421437fac in AcpiPsParseAml(ACPI_WALK_STATE*) ../../third_party/acpica/source/components/parser/psparse.c:550 <platform-bus-x86.so>+0x2b7fac #7 0x000021e4214464d2 in AcpiPsExecuteMethod(ACPI_EVALUATE_INFO*) ../../third_party/acpica/source/components/parser/psxface.c:244 <platform-bus-x86.so>+0x2c64d2 #8 0x000021e4213aa052 in AcpiNsEvaluate(ACPI_EVALUATE_INFO*) ../../third_party/acpica/source/components/namespace/nseval.c:250 <platform-bus-x86.so>+0x22a052 #9 0x000021e421413dd8 in AcpiNsInitOneDevice(ACPI_HANDLE, UINT32, void*, void**) ../../third_party/acpica/source/components/namespace/nsinit.c:735 <platform-bus-x86.so>+0x293dd8 #10 0x000021e421429e98 in AcpiNsWalkNamespace(ACPI_OBJECT_TYPE, ACPI_HANDLE, UINT32, UINT32, ACPI_WALK_CALLBACK, ACPI_WALK_CALLBACK, void*, void**) ../../third_party/acpica/source/components/namespace/nswalk.c:298 <platform-bus-x86.so>+0x2a9e98 #11 0x000021e4214131ac in AcpiNsInitializeDevices(UINT32) ../../third_party/acpica/source/components/namespace/nsinit.c:268 <platform-bus-x86.so>+0x2931ac #12 0x000021e42147c40d in AcpiInitializeObjects(UINT32) ../../third_party/acpica/source/components/utilities/utxfinit.c:304 <platform-bus-x86.so>+0x2fc40d #13 0x000021e42126d603 in acpi::AcpiImpl::InitializeAcpi(acpi::AcpiImpl*) ../../src/devices/board/lib/acpi/acpi-impl.cc:224 <platform-bus-x86.so>+0xed603 Add a simple check that avoids incrementing a pointer by zero, but otherwise behaves as before. Note that our findings are against ACPICA 20221020, but the same code exists on master.
diff --git a/source/components/dispatcher/dswstate.c b/source/components/dispatcher/dswstate.c
@@ -785,9 +785,14 @@ AcpiDsInitAmlWalk (
 
 
     WalkState->ParserState.Aml =
-    WalkState->ParserState.AmlStart = AmlStart;
+    WalkState->ParserState.AmlStart =
     WalkState->ParserState.AmlEnd =
-    WalkState->ParserState.PkgEnd = AmlStart + AmlLength;
+    WalkState->ParserState.PkgEnd = AmlStart;
+    /* Avoid undefined behavior: applying zero offset to null pointer */
+    if (AmlLength != 0) {
+      WalkState->ParserState.AmlEnd += AmlLength;
+      WalkState->ParserState.PkgEnd += AmlLength;
+    }
 
     /* The NextOp of the NextWalk will be the beginning of the method */
 
