Add forward stack offset support to stack value discovery

[sriemer]

maps: Get stack end as stack load address

Using the start of the stack region as load address does not provide
a useful match offset. The actual stack end is randomized within the
upper half of the stack region. But it can be read from
/proc/$pid/stat as decimal long unsigned at the 28th value (see
"man 5 proc").
So read it with fopen(), getline(), strtoul(), and a STRCHR() macro
based on a for-loop to avoid code duplication and actual calls to
strchr(). The issue with strchr() is that it does not have a
built-in and is slow this way. Handle errors gracefully and just
return the region start in case getting the stack end fails. Check
if the stack end is really a valid value within the stack region
bounds. Only support stacks growing with lower addresses for now.

handlers: list: Show proper forward stack offset

The game endless-sky has the credits value on the stack at around
0x640 forward stack offset. Ugtrain is able to freeze it and scanmem
should be able to discover it.
So add a check to the match offset calculation: If the matching
region is a stack region and the load address is greater than the
region start, then subtract the match address from the load address
as the load address must be the stack end then.

[sriemer]

V2: No need to set spos before reaching the stack end in the stat file.

diff --git a/maps.c b/maps.c
index 188431ffa9e6..7121ac8a4578 100644
--- a/maps.c
+++ b/maps.c
@@ -97,16 +97,16 @@ get_stack_end(pid_t target, unsigned long start, unsigned long end)
     }
 
     /* move to the end of the process name first */
-    spos = epos = line;
+    epos = line;
     STRCHR(
         if (*epos == '(') {
-            spos = epos++;
+            epos++;
             break;
         }
     );
     STRCHR(
         if (*epos == ')') {
-            spos = ++epos;
+            epos++;
             break;
         }
     );

[sriemer]

Verified by Coverity Scan, Valgrind, and disassembly.

Note: Coverity Scan does not detect memory leaks caused by getline() for the line buffer but Valgrind does. Also Valgrind detects further leaks related to libreadline usage but those might be false-positives (one time leaks required until exit and not cleaned up at exit).

[12345ieee]

I've had a preliminary look, some things:

• what are the impact of this on a scan? Am I correct in assuming this just changes the output shown and not the actual scan?
• given what you've changed I assume GC keeps working, I'll check if you haven't.
• the STRCHR macro that eats a "lambda" is a cool trick

[sriemer]

Yes, this only affects the output shown and only scanmem. There is a bit more work when reading the regions but only for the stack region. We get a suitable stack end as stack load address visible in lregions command and a suitable match offset.

GC is unaffected by the change. It only shows the new match offset in search results but doesn't make use of it. The load address is also ignored in GC.

I noticed an issue though: I thought that all data behind the stack end would be 0x0 but that is not the case. E.g. the environment variables are stored there. So matches behind the stack end should get a negative match offset: 0xffffxxxx to make it clearly visible that those are behind the stack end. I'd like to avoid introducing a -0x${offs} offset for those for now as this would require special parsing.

[sriemer]

My proposal is the following change for a V3:

diff --git a/handlers.c b/handlers.c
index 0e8770608154..a1176ef1c199 100644
--- a/handlers.c
+++ b/handlers.c
@@ -427,7 +427,7 @@ bool handler__list(globals_t *vars, char **argv, unsigned argc)
                   address_ul >= region_start) {
                     region_id = region->id;
                     if (region->type == REGION_TYPE_STACK &&
-                      region->load_addr > address_ul)
+                      region->load_addr > region_start)
                         match_off = region->load_addr - address_ul;
                     else
                         match_off = address_ul - region->load_addr;

[sriemer]

gnome-calculator can convert those if needed. Example:

twos FFFFFFFFFFFFE75E = 18A2

As we are usually interested in what is actually on the stack and not behind it, this should be a suitable method.

[sriemer]

Proposed change pushed as V3.

[sriemer]

Just rebased to current master.

[12345ieee]

Ok, I finally got around to testing it, I can confirm it works on the CLI.

The offsets beyond the stack break GC though, because they don't fit in an int64, I'll need to check if it's easy to fix, or we must introduce a negative offset for those.

Also, can you document somewhere (man page?) that the stack offset is computed backwards from the actual stack end and that ffff... offsets (or the -... offsets if we change them) mean beyond the stack?

[12345ieee]

Actually, GC is broken only under python 2, because there I use long, which is an int64.

In python3 I use the UINT64 provided by pygtk and everything is fine.

[sriemer]

Okay, will update the documentation and check how to fix GC.

[12345ieee]

The easiest way is to drop python2 support, and honestly I'm pretty tempted to do it.

[12345ieee]

This fixes it as well, for a very minor slowdown on py2:

diff --git a/gui/GameConqueror.py b/gui/GameConqueror.py
index 5386f60..d961e5f 100644
--- a/gui/GameConqueror.py
+++ b/gui/GameConqueror.py
@@ -1062,25 +1062,19 @@ class GameConqueror():
             self.scanresult_tv.set_model(None)
             # temporarily disable model for scanresult_liststore for the sake of performance
             self.scanresult_liststore.clear()
-            if misc.PY3K:
-                addr = GObject.Value(GObject.TYPE_UINT64)
-                off = GObject.Value(GObject.TYPE_UINT64)
+            addr = GObject.Value(GObject.TYPE_UINT64)
+            off = GObject.Value(GObject.TYPE_UINT64)
             line_regex = re.compile(r'^\[ *(\d+)\] +([\da-f]+), + \d+ \+ +([\da-f]+), +(\w+), (.*), +\[([\w ]+)\]$')
             for line in lines:
                 (mid_str, addr_str, off_str, rt, val, t) = line_regex.match(line).groups()
                 if t == 'unknown':
                     continue
                 mid = int(mid_str)
-                # `insert_with_valuesv` has the same function of `append`, but it's 7x faster
+                # `insert_with_valuesv` has the same function of `append`, but it's 5x faster
                 # PY3 has problems with int's, so we need a forced guint64 conversion
                 # See: https://bugzilla.gnome.org/show_bug.cgi?id=769532
-                # Still 5x faster even with the extra baggage
-                if misc.PY3K:
-                    addr.set_uint64(int(addr_str, 16))
-                    off.set_uint64(int(off_str, 16))
-                else:
-                    addr = long(addr_str, 16)
-                    off = long(off_str, 16)
+                addr.set_uint64(int(addr_str, 16))
+                off.set_uint64(int(off_str, 16))
                 self.scanresult_liststore.insert_with_valuesv(-1, [0, 1, 2, 3, 4, 5, 6], [addr, val, t, True, off, rt, mid])
                 # self.scanresult_liststore.append([addr, val, t, True, off, rt, mid])
             self.scanresult_tv.set_model(self.scanresult_liststore)

[sriemer]

Thanks, looks good.

[12345ieee]

I'll merge this in a few days, adding the GC support as another commit on top of this.
Maybe I can find a better way to not lose any speed in the GC part.