Fix NRE in AddUniversalDynamicSymbol for invalid symbol address ranges#2376
Fix NRE in AddUniversalDynamicSymbol for invalid symbol address ranges#2376brianrob merged 1 commit intomicrosoft:mainfrom
Conversation
brianrob
requested review from
StephenMolloy,
cincuranet,
leculver,
marklio and
mconnew
as code owners
| etlxStream.Position = 0; | ||
| using (var traceLog = new TraceLog(etlxStream)) | ||
| { | ||
| Assert.NotNull(traceLog); |
There was a problem hiding this comment.
This Assert is useless as traceLog isn't a value being returned from a method, it's an object being new'd up. The only way traceLog would be null is if the constructor throws, in which case the Assert statement wouldn't get hit.
There was a problem hiding this comment.
This is effectively a call to GC.KeepAlive. The goal is to make sure that the jit doesn't determine that the traceLog isn't used and eliminates the construction of the object entirely. I don't think it does that now, but I believe that would be allowed.
There was a problem hiding this comment.
I don't believe it would eliminate the call as the JIT isn't free to eliminate void returning method calls because the return value isn't used. And the return value is used here as the using statement is syntactic sugar for wrapping it in a try/finally block where traceLog.Dispose is called. There's also a bunch of criteria around what a JIT is allowed to optimize away. If any exceptions can be thrown by the method call (which a constructor call is a method call), then it's not allowed to optimize it away. Basically there are many reasons why the JIT can't optimize away the construction of TraceLog.
There was a problem hiding this comment.
Thanks. Pushed a change to address this.
When a ProcessSymbol event has an invalid address range (e.g., StartAddress=0x0, EndAddress=0xFFFFFFFFFFFFFFFF from zeroed /proc/kallsyms on Linux without root), the ETLX conversion crashed with a NullReferenceException. Three fixes applied: 1. Skip symbols where EndAddress <= StartAddress (invalid range) 2. Change (int) to (long) cast for the size parameter to match ForAllUnresolvedCodeAddressesInRange's parameter type 3. Add null guard on loadedModule as defense-in-depth Fixes microsoft#2373 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
brianrob
force-pushed
the
brianrob/issue-2373
branch
from
7d5380b to
c531c4c
Compare
2 participants
When a ProcessSymbol event has an invalid address range (e.g., StartAddress=0x0, EndAddress=0xFFFFFFFFFFFFFFFF from zeroed /proc/kallsyms on Linux without root), the ETLX conversion crashed with a NullReferenceException.
Three fixes applied:
Fixes #2373