Correct the handling of p_offset related to the program header in ckb… #49
Conversation
| @@ -413,7 +419,7 @@ int ckb_dlopen2(const uint8_t *dep_cell_hash, uint8_t hash_type, | |||
| const char *current_str = shrtab + sh->sh_name; | |||
| if (strcmp(DYNSTR, current_str) == 0) { | |||
| const uint8_t *addr2 = | |||
| addr_offset_checked(aligned_addr, aligned_size, sh->sh_offset); | |||
| addr_offset_checked(aligned_addr, aligned_size, sh->sh_addr); | |||
There was a problem hiding this comment.
Add reference or official documents about this changes.
There was a problem hiding this comment.
References mainly come from these places
In glibc, although sh_offset is used, it is read from the bin file here. In dlopen2, it reads from the memory that has been mapped.
And, it can be known from the official linux description that sh_addr is the address of the virtual memory, and sh_offset is the offset address of the binary file.
glibc code
linux elf in page: 1-11
Wiki linux elf
ckb_dlfcn.h
Outdated
| @@ -259,7 +259,13 @@ int ckb_dlopen2(const uint8_t *dep_cell_hash, uint8_t hash_type, | |||
| if (ph->p_offset < prepad) { | |||
| return ERROR_ELF_NOT_ALIGNED; | |||
| } | |||
| ret = _ckb_load_cell_code(addr2, memsz, ph->p_offset - prepad, ph->p_filesz, | |||
| uint64_t offset = ph->p_offset; | |||
There was a problem hiding this comment.
I wasn't following the logic here.
The original code does have a bug indeed, I also noticed it when I was tweaking other code. I believe the following code can already work:
ret = _ckb_load_cell_code(addr2, memsz, ph->p_offset - prepad, ph->p_filesz + prepad, index, CKB_SOURCE_CELL_DEP);
I wonder if there's anything else needed.
ckb_dlfcn.h
Outdated
| ret = _ckb_load_cell_code(addr2, memsz, ph->p_offset - prepad, ph->p_filesz, | ||
| uint64_t offset = ph->p_offset; | ||
| uint64_t read_size = ph->p_filesz; | ||
| if (offset % RISCV_PGSIZE != 0) { |
There was a problem hiding this comment.
Do we really need to align file offset? I personally wasn't sure if there is a need for that.
There was a problem hiding this comment.
Actually, this change is identical to :
ret = _ckb_load_cell_code(addr2, memsz, ph->p_offset - prepad, ph->p_filesz + prepad, index, CKB_SOURCE_CELL_DEP);
The same padding is calculated, twice.
There was a problem hiding this comment.
In that sense I don't think we need this bulk of changes, we have prepad above, which is also used in a condition, we can simply make the one line change, there is no need to calculate the padding again here.
There was a problem hiding this comment.
The prepad is calculated from p_vaddr. As we know, p_vaddr and p_offset should satisfy:
p_vaddr - p_offset % align == 0, which align can be 4096, or 2048, ... even 1, depending or compiler or linker script.
When align is not 4096, these paddings are not same.
There was a problem hiding this comment.
We could debate the formula used to calculate padding, but for one library, there should be only one padding used, so I would suggest either use prepad and delete the rest, or keep current implementation, and delete parts related to prepad. Current code, IMHO, is slightly messy.
There was a problem hiding this comment.
Correct: When padding are not same, we should take original prepad: p_vaddr % RISCV_PGSIZE.
|
If you want to calculate padding in the correct way, you should remove code related to |
No description provided.