From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/1654 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: ldso : dladdr support Date: Sun, 19 Aug 2012 22:06:26 -0400 Message-ID: <20120820020626.GD27715@brightrain.aerifal.cx> References: <20120807114627.GG30810@port70.net> <50212306.6070402@gmail.com> <20120807230933.GC27715@brightrain.aerifal.cx> <502237A1.1000805@gmail.com> <20120808115202.GL30810@port70.net> <5022703B.3090105@gmail.com> <20120811230536.GQ27715@brightrain.aerifal.cx> <20120817053934.GS27715@brightrain.aerifal.cx> <50311776.9040802@gmail.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii X-Trace: ger.gmane.org 1345428307 27899 80.91.229.3 (20 Aug 2012 02:05:07 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 20 Aug 2012 02:05:07 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-1655-gllmg-musl=m.gmane.org@lists.openwall.com Mon Aug 20 04:05:07 2012 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1T3HMZ-00048e-CF for gllmg-musl@plane.gmane.org; Mon, 20 Aug 2012 04:05:07 +0200 Original-Received: (qmail 1410 invoked by uid 550); 20 Aug 2012 02:05:05 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 1401 invoked from network); 20 Aug 2012 02:05:05 -0000 Content-Disposition: inline In-Reply-To: <50311776.9040802@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Xref: news.gmane.org gmane.linux.lib.musl.general:1654 Archived-At: On Sun, Aug 19, 2012 at 06:42:30PM +0200, musl wrote: > Hi, > > This patch fixes a bug in dladdr: sym var was not incremented across gnu hash chain iteration). > I also reworked the dladdr implem to share more code between sysv and gnu hash. > I still haven't found a better way to get the symbol table size. Do you? > > This patch uses the new decode_vec function, but as I told you in my > previous mail, I'm not sure this the way to go. > Could you tell me what you think? Yeah, I'm not really happy with it either. Trying to think of something better... > diff --git a/src/ldso/dynlink.c b/src/ldso/dynlink.c > index f55c6f1..bf1ec6b 100644 > --- a/src/ldso/dynlink.c > +++ b/src/ldso/dynlink.c > @@ -1,3 +1,4 @@ > +#define _GNU_SOURCE > #include > #include > #include > @@ -28,12 +29,14 @@ typedef Elf32_Phdr Phdr; > typedef Elf32_Sym Sym; > #define R_TYPE(x) ((x)&255) > #define R_SYM(x) ((x)>>8) > +#define ELF_ST_TYPE ELF32_ST_TYPE > #else > typedef Elf64_Ehdr Ehdr; > typedef Elf64_Phdr Phdr; > typedef Elf64_Sym Sym; > #define R_TYPE(x) ((x)&0xffffffff) > #define R_SYM(x) ((x)>>32) > +#define ELF_ST_TYPE ELF64_ST_TYPE > #endif These definitions are actually the same. I would just #define ST_TYPE(x) ((x)&15) > -static uint32_t hash(const char *s0) > +static uint32_t sysv_hash(const char *s0) > { > const unsigned char *s = (void *)s0; > uint_fast32_t h = 0; > @@ -105,7 +117,16 @@ static uint32_t hash(const char *s0) > return h & 0xfffffff; > } > > -static Sym *lookup(const char *s, uint32_t h, struct dso *dso) > +static uint32_t gnu_hash (const char *s0) > +{ > + const unsigned char *s = (void *)s0; > + uint_fast32_t h = 5381; > + for (; *s; s++) > + h = h*33 + *s; > + return h & 0xffffffff; > +} The final &0xffffffff is a no-op. Note that the one in sysv_hash is not a no-op; sysv_hash's result is 28 bits, not 32. Re-reading this code also raised another issue: I'm not entirely convinced that 0 is not a possible hash value, which may invalidate what I said before about using h==0 to indicate "not yet computed". Of course, it may not matter; if one in 4 billion symbol names get their hashes repeatedly recomputed rather than being reused, it's not going to make any difference to overall performance... > /* Only trust user/env if kernel says we're not suid/sgid */ > - if ((aux[0]&0x7800)!=0x7800 || aux[AT_UID]!=aux[AT_EUID] > - || aux[AT_GID]!=aux[AT_EGID] || aux[AT_SECURE]) { > + if ((found&0x1e0)!=0x1e0 || aux[5]!=aux[6] > + || aux[7]!=aux[8] || aux[9]) { > env_path = 0; > env_preload = 0; Looking at this, I agree that the new decode_vec idea is not a good direction. It's obfuscating the code badly. For now, how about leaving the old decode_vec alone and just adding a new one with a different name for getting to "high" entries. I wonder if it would be possible, rather than using a list of wanted entries, to use a base/count rather than always working zero-based like decode_vec does. This would allow the resulting indices to still actually mean something so we don't wind up with magic numbers all over the code.. Rich