The DLAStep SimplifyInstanceAtOffset0 now only triggers if the parent
node has the child-at-offset-0 node as its only successor, or when the
child-at-offset-0 doesn't have other predecessors.
Doing this guarantees that it's impossible for another predecessor of
the child-at-offset-0 to start seeing memory accesses that were
initially relative to the parent.
This condition is slightly more restrictive than the previous one, but
it takes into consideration some far reaching consequences.
If SimplifyInstanceAtOffset0 aggressively like we did before, DLA can
end up inferring types in some memory locations, like executable
segments, for which there aren't clues in the binary.
This isn't bad per se, but if DLA does that, the newly recovered type is
identified as non-executable data, causing misdecompilation because
rev.ng doesn't decompile memory regions that it understands as non
executable.
This commit, making SimplifyInstanceAtOffset0 less aggressive, makes it
play better with the rest of the assumptions of the decompilation
pipeline.
It also relaxes a decompilation test that was previously working by
chance and that was effectively beyond the current expressive power of
reasoning for DLA at the moment.
Before this commit the breadth-first traversal was more convoluted and
it used a broken criterion for bailing out, resulting sometimes in
considering a pointer edge as if it was an instance edge.
This in turn caused bugs to the point of the DLA Graph failing
verification.
This commit rewrites and simplifies the main traversal and comparisons
in DeduplicateField to make them robust against these bugs.
Before this commit the code was wrong in 2 ways:
- if failed to iterate on children, because it iterated a range that was
effectively [begin, begin) instead of [begin, end)
- the update of the ARootNext iterator in the loop was broken, but it
was impossible to realize that since the loop was never taken
This commit fixes both these intertwined problems.
Before this commit, the DLAStep MergePointeesOfPointerUnion wasn't
really equipped for dealing with LayoutTypeSystemNodes representing
types imported from Model.
There was some code that tried to deal with them but it was mostly an
afterthought and it wasn't robust.
This commit fixes that, and is able to handle a strict superset of the
scenarios envisioned before, while preserving the property that nodes
representing types imported from Model should be preserved (in
particular their size).
Before this commit, DLA didn't do any checks on the validity of the
model::PrimitiveTypes that it generated.
This could cause invalid PrimitiveTypes to be generated, with weird
sizes not supported by the Model.
This commit fixes this problem, generating empty model::StructTypes with
the proper sizes instead.
Before this commit, DLA's frontend was representing information about
string literals in a way that had a bad outcome: very often, all (or
most) places in the binary that were using string literals ended up
collapsed on the DLA graph on the same node, causing all their types to
be a weird struct.
This was wrong, and it was the result of an aggressive creation of
equality edges.
This commit avoids to emit equality edges, and replaces them with
instance edges, that represent the fact that a call to StringLiteral
actually returns a type that represents a pointer to char.
Before this commit, the method could partly fail in moving the edge
target even in legitimate scenarios, because of a bug in how it fixed up
the Successors link in the edge source.
This commit fixes the bug.
Now CompactCompatibleArrays runs after ArrangeAccessesHierarchically.
Rearranging the accesses hierarchically first allows the following step,
that compacts compatible arrays, to achieve better results, and overall
recover much better looking arrays.
This commit extends the CompactCompatibleArrays DLAStep to also consider
non-strided accesses.
Strided accesses are still always considered first, and they are still
considered the only real source of information on arrays, but after
having tried to compact all the compatible arrays,
CompactCompatibleArrays now also considers non-strided instance edges to
see if they can be compacted with the rest of the inferred array.
This has shown to handle gracefully a number of real-world examples and
reduce unions.
Before this commit, the CompactCompatibleArray DLAStep had that caused
the compaction to be sensible to the order of the pairs of edges that
were compacted, and that also caused the compaction operation to overrun
the end of the containing struct in some corner cases.
This commit fixes both bugs.
Now the compaction routine is not ordering-sensitive anymore.
For each pair of edges <A, B> to compact it tries to compact them in 2
possible ways: by aligning A to B shifting A to lower offsets, and by
aligning B to A shifting B to lower offsets.
If both succeed, it picks the best result among the two, which doesn't
depend on the ordering of A and B.
This fixes the order-sensitiveness.
While reworking this logic, the logic was fixed so that if any of the
two compaction attempts causes to overrun the end of the containing
struct, the attempt is not considered successful anymore, and it's
discarded altogether.
The inter-procedural part of the DLA frontend connects actual arguments
of function calls with formal arguments of the callee functions, with
instance links at offset 0.
This represents the information that the type pointed-to by the actual
argument has an instance of the type pointed-to by the formal argument
at offset 0.
Before this commit, this was done even when the actual argument was an
integer constant leading to various problems:
1. Most of the times, small integer constants passed as actual
cannot represent valid pointers, leading to graphs bigger than
necessary.
2. Even when an integer constant might actually represent a pointer into
valid memory, if it does it should fall into some segment.
If it does, by the time DLA runs, those integer constants should have
already converted into calls to the special SegmentRef opcode, which
is already handled properly.
3. Finally, those constants end up being very connected in the DLA
graph, creating connections between other nodes that are otherwise
unrelated or very far from each other. This pollutes the graph and
rapidly degrades the quality of the results.
This commit properly guards the code so that the instance-at-offset-0
link between actual and formal arguments is never injected if the actual
argument is an integer constant.
Before this commit, DLA's backend often emitted things like:
```c
struct x {
struct y {
int32_t z;
uint8_t padding[4];
} _offset_0;
uint8_t padding[8];
};
```
This is suboptimal, since the fields of y actually can just be inlined
into x, giving:
```c
struct x {
int32_t z;
uint8_t padding[12];
};
```
This behaviour was due to a logic bug carried over from old versions of
the code, and partially extended, but never revisited nor thought
through.
This commit fixes the problem, making the second behaviour the only
thing DLA ever does when updating the model.
Basically, struct fields are copied over field by field, instead of
inserting the bulk struct (y in the example) inside the larger struct to
update (x in the example).
Alessandro Di Federico