Re: [PATCH] fix lookup of ldso's dynamic section for GOLD linker

Re: [PATCH] fix lookup of ldso's dynamic section for GOLD linker

[Rich Felker]

[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]

On Thu, Jan 19, 2012 at 05:53:10PM +0100, retnyg@gmx.net wrote:
> here's a better one with only the necessary stuff.
> also it doesnt use sizeof but the correct value of each program
> header as stored in the elf header

Looks roughly correct now and it's clear that it's not changing
anything about load order. Thanks!

> >From 26754baff1ee5c5a8001c0b3ed5faddcd58b8ca3 Mon Sep 17 00:00:00 2001
> From: rofl0r <retnyg@gmx.net>
> Date: Thu, 19 Jan 2012 14:54:39 +0100
> Subject: [PATCH] fix lookup of ldso's dynamic section for GOLD linker
> 
> the previous code worked only with the ld linker.
> with the old technique, lib_dyn[DT_STRTAB] et al were 0.
> ---
>  src/ldso/dynlink.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
> index 86d4b80..ff12a78 100644
> --- a/src/ldso/dynlink.c
> +++ b/src/ldso/dynlink.c
> @@ -498,8 +498,18 @@ void *__dynlink(int argc, char **argv, size_t *got)
>  		}
>  	}
>  
> +	got[0] += aux[AT_BASE];
> +	

Note that got[0] is still used later as the dynamic vector address,
but your patch does not set it correctly when the original value was
not the load-address-relative pointer to it...

>  	/* Relocate ldso's DYNAMIC pointer and load vector */
> -	decode_vec((void *)(got[0] += aux[AT_BASE]), lib_dyn, DYN_CNT);
> +	ehdr = (void*) aux[AT_BASE];
> +	phdr = (Phdr*) ((char*) ehdr + ehdr->e_phoff);
> +	
> +	for(i = 0; i < ehdr->e_phnum; i++, phdr = (void *)((char *) phdr + ehdr->e_phentsize)) {
> +		if (phdr->p_type == PT_DYNAMIC) {
> +			decode_vec((void *) ((char*) ehdr + phdr->p_vaddr), lib_dyn, DYN_CNT);
> +			break;
> +		}
> +	}

This looks correct. I'm making some relatively minor cosmetic changes.
By the way, in the future please attach patches rather than sending
them inline so the tabs don't get ruined, lines don't get wrapped,
etc. Mail bodies are generally not patch-clean.

See the attached patch and let me know if it looks ok..

Rich

[-- Attachment #2: gold-issue.diff --]
[-- Type: text/plain, Size: 1412 bytes --]

diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
index 86d4b80..b26992d 100644
--- a/src/ldso/dynlink.c
+++ b/src/ldso/dynlink.c
@@ -467,6 +467,7 @@ void *__dynlink(int argc, char **argv, size_t *got)
 	size_t i;
 	Phdr *phdr;
 	Ehdr *ehdr;
+	size_t *lib_dynv;
 	static struct dso builtin_dsos[3];
 	struct dso *const app = builtin_dsos+0;
 	struct dso *const lib = builtin_dsos+1;
@@ -498,8 +499,16 @@ void *__dynlink(int argc, char **argv, size_t *got)
 		}
 	}
 
-	/* Relocate ldso's DYNAMIC pointer and load vector */
-	decode_vec((void *)(got[0] += aux[AT_BASE]), lib_dyn, DYN_CNT);
+	/* Find the dynamic linker's DYNAMIC section and decode it */
+	ehdr = (void *)aux[AT_BASE];
+	phdr = (void *)(aux[AT_BASE] + ehdr->e_phoff);
+	for (i=ehdr->e_phnum; i--; phdr=(void *)((char *)phdr + ehdr->e_phentsize)) {
+		if (phdr->p_type == PT_DYNAMIC) {
+			lib_dynv = (void *)(aux[AT_BASE] + phdr->p_vaddr);
+			decode_vec(lib_dynv, lib_dyn, DYN_CNT);
+			break;
+		}
+	}
 
 	/* Find the program image's DYNAMIC section and decode it */
 	phdr = (void *)aux[AT_PHDR];
@@ -526,7 +535,7 @@ void *__dynlink(int argc, char **argv, size_t *got)
 		.strings = (void *)(aux[AT_BASE]+lib_dyn[DT_STRTAB]),
 		.hashtab = (void *)(aux[AT_BASE]+lib_dyn[DT_HASH]),
 		.syms = (void *)(aux[AT_BASE]+lib_dyn[DT_SYMTAB]),
-		.dynv = (void *)(got[0]),
+		.dynv = lib_dynv,
 		.name = "libc.so",
 		.global = 1,
 		.relocated = 1

Re: Re: [PATCH] fix lookup of ldso's dynamic section for GOLD linker

[gs]

On 01/20/2012 04:20 AM, Rich Felker wrote:
> On Thu, Jan 19, 2012 at 05:53:10PM +0100, retnyg@gmx.net wrote:
>> here's a better one with only the necessary stuff.
>> also it doesnt use sizeof but the correct value of each program
>> header as stored in the elf header
> Looks roughly correct now and it's clear that it's not changing
> anything about load order. Thanks!
>
>> > From 26754baff1ee5c5a8001c0b3ed5faddcd58b8ca3 Mon Sep 17 00:00:00 2001
>> From: rofl0r<retnyg@gmx.net>
>> Date: Thu, 19 Jan 2012 14:54:39 +0100
>> Subject: [PATCH] fix lookup of ldso's dynamic section for GOLD linker
>>
>> the previous code worked only with the ld linker.
>> with the old technique, lib_dyn[DT_STRTAB] et al were 0.
>> ---
>>   src/ldso/dynlink.c |   12 +++++++++++-
>>   1 files changed, 11 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c
>> index 86d4b80..ff12a78 100644
>> --- a/src/ldso/dynlink.c
>> +++ b/src/ldso/dynlink.c
>> @@ -498,8 +498,18 @@ void *__dynlink(int argc, char **argv, size_t *got)
>>   		}
>>   	}
>>
>> +	got[0] += aux[AT_BASE];
>> +	
> Note that got[0] is still used later as the dynamic vector address,
> but your patch does not set it correctly when the original value was
> not the load-address-relative pointer to it...
i wasn't sure about what it is used for, and since your original code 
increased it, i just did the same.
your new code now does not use "got" anywhere.
if that's ok, maybe (void) it; so that the compiler does not warn about 
unused parameters...
>>   	/* Relocate ldso's DYNAMIC pointer and load vector */
>> -	decode_vec((void *)(got[0] += aux[AT_BASE]), lib_dyn, DYN_CNT);
>> +	ehdr = (void*) aux[AT_BASE];
>> +	phdr = (Phdr*) ((char*) ehdr + ehdr->e_phoff);
>> +	
>> +	for(i = 0; i<  ehdr->e_phnum; i++, phdr = (void *)((char *) phdr + ehdr->e_phentsize)) {
>> +		if (phdr->p_type == PT_DYNAMIC) {
>> +			decode_vec((void *) ((char*) ehdr + phdr->p_vaddr), lib_dyn, DYN_CNT);
>> +			break;
>> +		}
>> +	}
> This looks correct. I'm making some relatively minor cosmetic changes.
> By the way, in the future please attach patches rather than sending
> them inline so the tabs don't get ruined, lines don't get wrapped,
> etc. Mail bodies are generally not patch-clean.
>
> See the attached patch and let me know if it looks ok..
>
> Rich
your patch looks ok and works well here, with both ld and gold and even 
with LD_PRELOAD.
unsure about PIE though.