CC'ing Jan Martin Milata <[email protected]> writes:
> Hi, > I don't have much to say regarding what the script does -- I think it's > a good start and you probably have better knowledge of low-level details > than I do anyway. Perhaps someone from security or tools department > might give us some feedback or hint how to go about this? There's also > similar script for the ubuntu error reporting tool, you might want to > check it out for some ideas: > http://bazaar.launchpad.net/~apport-hackers/apport/trunk/view/head:/data/general-hooks/parse_segv.py > > As the script only uses the instruction pointer and signal number from > the coredump, we could run it on the server side in the future, as both > values should be available in a uReport. The major advantages of this > approach would be that we don't have to depend on GDB (apparently a > problem on RHEL) and that we can run improved versions of the script on > crashes received when this version was not available. The plugin doesn't > seem to be integrated with GDB very tightly, so this should be easy if > we keep this in mind. > > Some remarks regarding the code below: > > On Thu, Jun 13, 2013 at 14:01:11 +0200, Denys Vlasenko wrote: >> +_writing_instr = { >> + # insn:N, where N: >> + # -1: this insn always writes to memory >> + # -2: writes to memory if any operand is a memory operand >> + # 2: writes to memory if 2nd (or later) operand is a memory operand >> + # >> + # Two-operand insns >> + "add":2, > I suggest using something more descriptive, like assigning the numbers > to a variable, e.g. WRITES_ALWAYS = -1, and then using those. > >> +def _fetch_insn_from_table(ins, table): >> + if not ins: >> + return None >> + if ins in table.keys(): >> + return table[ins] > You can use just "ins in table". > >> + if self.operands.startswith("0x") or self.operands.startswith("-0x") or >> self.operands.startswith("("): >> + mem_op_pos = 0 >> + else: >> + mem_op_pos = self.operands.find(",0x") >> + if mem_op_pos < 0: >> + mem_op_pos = self.operands.find(",-0x") >> + if mem_op_pos < 0: >> + mem_op_pos = self.operands.find(":0x") >> + if mem_op_pos < 0: >> + mem_op_pos = self.operands.find(":-0x") >> + if mem_op_pos < 0: >> + mem_op_pos = self.operands.find(",(") >> + if mem_op_pos < 0: >> + mem_op_pos = self.operands.find(":(") >> + if mem_op_pos < 0: >> + return False # no memory operands >> + mem_op_pos += 1 > Can you use a regular expression for this, possibly with splitting the > operands into list first? http://docs.python.org/2.7/library/re.html > >> +class _Signal_and_Insn: >> + pass > As Jiri said, I'd make most of the functions method of this class. At > least you'll get rid of the underscores before their names;) > >> +def _get_signal_and_instruction(): > This one should probably be the constructor. As mentioned, I'd probably > move calls to the gdb module outside the class (but that can be done > when we actually need it). > >> + # HACK_ALERT: kernels before 3.?.? do not record siginfo in >> coredumps, >> + # so $_siginfo isn't present. > By the way, do you have any reference for this? The prstatus structure > which contains a signal number seems to be present in coredumps at least > on 2.6.32. Or am I misunderstanding the problem? > >> + for line in raw_instructions.split("\n"): >> + # line can be: >> + # "Dump of assembler code from 0xAAAA to 0xBBBB:" >> + # "[=>] 0x00000000004004dc[ <+0>]: push %rbp" >> + # (" <+0>" part is present when we run on a live process, >> + # on coredump it is absent) >> + # "End of assembler dump." >> + # "" (empty line) > What about using regular expressions here? > >> + elif self.signo == signal.SIGQUIT: >> + self.exploitable_rating = 0 >> + self.exploitable_desc = "QUIT signal (Ctrl-\ pressed?)" > The backslash should probably be doubled or the whole string literal > preceded by the letter r. This was found by pylint, I can recommend > running it yourself, it can be useful sometimes. > >> + elif self.signo == signal.SIGBUS: >> + self.exploitable_rating = 5 >> + self.exploitable_desc = "Access past the end of mapped file, >> invalid address, unaligned access, etc" >> + #elif self.signo = signal.SIGfoo: > The giant if-else block looks ugly, but I can't think of a less ugly way > to replace it ... > >> + f = sys.stdout > The sys module is not imported (says pylint). > > > Cheers, > Martin -- Nikola
