gramine icon indicating copy to clipboard operation
gramine copied to clipboard

[Pal,LibOS] Debug-related functions incorrectly call the argument `addr` (should be `base_diff`)

Open dimakuv opened this issue 4 years ago • 4 comments

Description of the problem

In PR #255, we decided to rename all l_addr to l_base_diff. This is because the ELF standard confusingly uses addr to denote the "difference between the virtual addresses in the ELF file and the actual virtual addresses in memory". So this confusing name addr propagated to the debug-related functions: remove_r_debug, append_r_debug, shim_describe_location, DkDebugMapAdd, DkDebugMapRemove, DkDebugDescribeLocation and so on.

We need to rename addr argument of these functions to base_diff, to be less confusing and to be uniform with RTLD code.

Also, there seems to be a bug in at least DkDebugDescribeLocation implementation -- in some places it assumes that the address given to it is an actual address in memory, whereas g_debug_map actually stores base diff addresses. So for example, DkDebugDescribeLocation doesn't work for ET_EXEC ELF objects (at least this is my intuition).

@pwmarcz Any comments on this?

dimakuv avatar Dec 02 '21 10:12 dimakuv

Also, there seems to be a bug in at least DkDebugDescribeLocation implementation -- in some places it assumes that the address given to it is an actual address in memory, whereas g_debug_map actually stores base diff addresses. So for example, DkDebugDescribeLocation doesn't work for ET_EXEC ELF objects (at least this is my intuition).

I'm not sure why it wouldn't work for these. Based on what I know, an ET_EXEC file is passed to DkDebugDescribeLocation with "address" (i.e. base diff) of 0, which means that the addresses in memory will directly correspond to addresses in the *.map file that we produce.

If you can reproduce the problem, maybe you could look a bit deeper:

  • What's the reported RIP
  • Does it correspond to a function in the ELF file (based on objdump -d)
  • Does it correspond to an entry in the *.map file for that ELF file

pwmarcz avatar Dec 02 '21 10:12 pwmarcz

This is because the ELF standard confusingly uses addr to denote the "difference between the virtual addresses in the ELF file and the actual virtual addresses in memory".

I would like to point out, that the meaning of this base address is probably an address where the "virtual" ELF starts, but there might be nothing mapped there (if first load segment has vaddr != 0). This name makes some sense, but is quite confusing for the readers.

boryspoplawski avatar Dec 02 '21 11:12 boryspoplawski

Ah, so DkDebugDescribeLocation() also works on "base diffs", not actual addresses? I read the code wrong then.

Anyway, this only shows how confusing this addr word is.

Btw, is this intended? Like, we can't have an EXEC file (its addr == 0x0) and a DYN file mapped at 0x0 (it is technically possible). Only one of them will be found in g_debug_map.

dimakuv avatar Dec 02 '21 11:12 dimakuv

Btw, is this intended? Like, we can't have an EXEC file (its addr == 0x0) and a DYN file mapped at 0x0 (it is technically possible). Only one of them will be found in g_debug_map.

Right, the code makes an assumption that addr is unique in several places. I think it's fixable. The biggest problem is that DkDebugMapRemove takes just addr, without file path.

pwmarcz avatar Dec 02 '21 11:12 pwmarcz