cmd/ld: fix symbol table sorting

runtime: double-check that symbol table is sorted

If the symbol table is unsorted, the binary search in findfunc
will not find its func, which will make stack traces stop early.
When the garbage collector starts using the stack tracer,
that would be a serious problem.

The unsorted symbol addresses came from from two things:

1. The symbols in an ELF object are not necessarily sorted,
   so sort them before adding them to the symbol list.

2. The __i686.get_pc_thunk.bx symbol is present in multiple
   object files and was having its address adjusted multiple
   times, producing an incorrect address in the symbol table.

R=golang-dev, iant
CC=golang-dev
https://golang.org/cl/7440044

diff --git a/src/cmd/ld/data.c b/src/cmd/ld/data.c
index 1e0bd2cd0fec7119a01f72fea7b8f1dd4d8058eb..6c6b1be433582178477adaaaae960180efbfd78f 100644 (file)
--- a/src/cmd/ld/data.c
+++ b/src/cmd/ld/data.c
@@ -58,70 +58,73 @@ datcmp(Sym *s1, Sym *s2)
 }
 
 Sym*
-datsort(Sym *l)
+listsort(Sym *l, int (*cmp)(Sym*, Sym*), int off)
 {
        Sym *l1, *l2, *le;
+       #define NEXT(l) (*(Sym**)((char*)(l)+off))
 
-       if(l == 0 || l->next == 0)
+       if(l == 0 || NEXT(l) == 0)
                return l;
 
        l1 = l;
        l2 = l;
        for(;;) {
-               l2 = l2->next;
+               l2 = NEXT(l2);
                if(l2 == 0)
                        break;
-               l2 = l2->next;
+               l2 = NEXT(l2);
                if(l2 == 0)
                        break;
-               l1 = l1->next;
+               l1 = NEXT(l1);
        }
 
-       l2 = l1->next;
-       l1->next = 0;
-       l1 = datsort(l);
-       l2 = datsort(l2);
+       l2 = NEXT(l1);
+       NEXT(l1) = 0;
+       l1 = listsort(l, cmp, off);
+       l2 = listsort(l2, cmp, off);
 
        /* set up lead element */
-       if(datcmp(l1, l2) < 0) {
+       if(cmp(l1, l2) < 0) {
                l = l1;
-               l1 = l1->next;
+               l1 = NEXT(l1);
        } else {
                l = l2;
-               l2 = l2->next;
+               l2 = NEXT(l2);
        }
        le = l;
 
        for(;;) {
                if(l1 == 0) {
                        while(l2) {
-                               le->next = l2;
+                               NEXT(le) = l2;
                                le = l2;
-                               l2 = l2->next;
+                               l2 = NEXT(l2);
                        }
-                       le->next = 0;
+                       NEXT(le) = 0;
                        break;
                }
                if(l2 == 0) {
                        while(l1) {
-                               le->next = l1;
+                               NEXT(le) = l1;
                                le = l1;
-                               l1 = l1->next;
+                               l1 = NEXT(l1);
                        }
                        break;
                }
-               if(datcmp(l1, l2) < 0) {
-                       le->next = l1;
+               if(cmp(l1, l2) < 0) {
+                       NEXT(le) = l1;
                        le = l1;
-                       l1 = l1->next;
+                       l1 = NEXT(l1);
                } else {
-                       le->next = l2;
+                       NEXT(le) = l2;
                        le = l2;
-                       l2 = l2->next;
+                       l2 = NEXT(l2);
                }
        }
-       le->next = 0;
+       NEXT(le) = 0;
        return l;
+       
+       #undef NEXT
 }
 
 Reloc*
@@ -1010,7 +1013,7 @@ dodata(void)
                                s->type = SDATARELRO;
                }
        }
-       datap = datsort(datap);
+       datap = listsort(datap, datcmp, offsetof(Sym, next));
 
        /*
         * allocate sections.  list is sorted by type,

diff --git a/src/cmd/ld/ldelf.c b/src/cmd/ld/ldelf.c
index 19c582b00745e0da8b0d28966dbc62df077dcd36..2bbf4f83e356b7d0ee5d689c20cafebb1d40a10f 100644 (file)
--- a/src/cmd/ld/ldelf.c
+++ b/src/cmd/ld/ldelf.c
@@ -311,6 +311,16 @@ static int map(ElfObj*, ElfSect*);
 static int     readsym(ElfObj*, int i, ElfSym*, int);
 static int     reltype(char*, int, uchar*);
 
+int
+valuecmp(Sym *a, Sym *b)
+{
+       if(a->value < b->value)
+               return -1;
+       if(a->value > b->value)
+               return +1;
+       return 0;
+}
+
 void
 ldelf(Biobuf *f, char *pkg, int64 len, char *pn)
 {
@@ -541,13 +551,6 @@ ldelf(Biobuf *f, char *pkg, int64 len, char *pn)
                }
                s->size = sect->size;
                s->align = sect->align;
-               if(s->type == STEXT) {
-                       if(etextp)
-                               etextp->next = s;
-                       else
-                               textp = s;
-                       etextp = s;
-               }
                sect->sym = s;
        }
 
@@ -583,6 +586,12 @@ ldelf(Biobuf *f, char *pkg, int64 len, char *pn)
                        continue;
                }
                s = sym.sym;
+               if(s->outer != S) {
+                       if(s->dupok)
+                               continue;
+                       diag("%s: duplicate symbol reference: %s in both %s and %s", pn, s->name, s->outer->name, sect->sym->name);
+                       errorexit();
+               }
                s->sub = sect->sym->sub;
                sect->sym->sub = s;
                s->type = sect->sym->type | (s->type&~SMASK) | SSUB;
@@ -611,7 +620,25 @@ ldelf(Biobuf *f, char *pkg, int64 len, char *pn)
                                p->link = nil;
                                p->pc = pc++;
                                s->text = p;
-
+                       }
+               }
+       }
+       
+       // Sort outer lists by address, adding to textp.
+       // This keeps textp in increasing address order.
+       for(i=0; i<obj->nsect; i++) {
+               s = obj->sect[i].sym;
+               if(s == S)
+                       continue;
+               if(s->sub)
+                       s->sub = listsort(s->sub, valuecmp, offsetof(Sym, sub));
+               if(s->type == STEXT) {
+                       if(etextp)
+                               etextp->next = s;
+                       else
+                               textp = s;
+                       etextp = s;
+                       for(s = s->sub; s != S; s = s->sub) {
                                etextp->next = s;
                                etextp = s;
                        }
@@ -792,7 +819,7 @@ readsym(ElfObj *obj, int i, ElfSym *sym, int needSym)
                                // set dupok generally. See http://codereview.appspot.com/5823055/
                                // comment #5 for details.
                                if(s && sym->other == 2) {
-                                       s->type = SHIDDEN;
+                                       s->type |= SHIDDEN;
                                        s->dupok = 1;
                                }
                        }
@@ -804,14 +831,14 @@ readsym(ElfObj *obj, int i, ElfSym *sym, int needSym)
                                        // and should only reference by its index, not name, so we
                                        // don't bother to add them into hash table
                                        s = newsym(sym->name, version);
-                                       s->type = SHIDDEN;
+                                       s->type |= SHIDDEN;
                                }
                        break;
                case ElfSymBindWeak:
                        if(needSym) {
                                s = newsym(sym->name, 0);
                                if(sym->other == 2)
-                                       s->type = SHIDDEN;
+                                       s->type |= SHIDDEN;
                        }
                        break;
                default:

diff --git a/src/cmd/ld/ldmacho.c b/src/cmd/ld/ldmacho.c
index 3310903e18e341162f995aa652c3e5e77e8dd0bc..41852f17c67280f4335242fe1c0099150ac19c3e 100644 (file)
--- a/src/cmd/ld/ldmacho.c
+++ b/src/cmd/ld/ldmacho.c
@@ -593,13 +593,6 @@ ldmacho(Biobuf *f, char *pkg, int64 len, char *pn)
                        } else
                                s->type = SDATA;
                }
-               if(s->type == STEXT) {
-                       if(etextp)
-                               etextp->next = s;
-                       else
-                               textp = s;
-                       etextp = s;
-               }
                sect->sym = s;
        }
        
@@ -631,6 +624,12 @@ ldmacho(Biobuf *f, char *pkg, int64 len, char *pn)
                        werrstr("reference to invalid section %s/%s", sect->segname, sect->name);
                        continue;
                }
+               if(s->outer != S) {
+                       if(s->dupok)
+                               continue;
+                       diag("%s: duplicate symbol reference: %s in both %s and %s", pn, s->name, s->outer->name, sect->sym->name);
+                       errorexit();
+               }
                s->type = outer->type | SSUB;
                s->sub = outer->sub;
                outer->sub = s;
@@ -661,11 +660,29 @@ ldmacho(Biobuf *f, char *pkg, int64 len, char *pn)
                        p->link = nil;
                        p->pc = pc++;
                        s->text = p;
+               }
+               sym->sym = s;
+       }
 
-                       etextp->next = s;
+       // Sort outer lists by address, adding to textp.
+       // This keeps textp in increasing address order.
+       for(i=0; i<c->seg.nsect; i++) {
+               sect = &c->seg.sect[i];
+               if((s = sect->sym) == S)
+                       continue;
+               if(s->sub)
+                       s->sub = listsort(s->sub, valuecmp, offsetof(Sym, sub));
+               if(s->type == STEXT) {
+                       if(etextp)
+                               etextp->next = s;
+                       else
+                               textp = s;
                        etextp = s;
+                       for(s = s->sub; s != S; s = s->sub) {
+                               etextp->next = s;
+                               etextp = s;
+                       }
                }
-               sym->sym = s;
        }
 
        // load relocations

diff --git a/src/cmd/ld/ldpe.c b/src/cmd/ld/ldpe.c
index 8923bc729cc9a31e34226e2c9749d3ffceb9a577..f41827befda6f9f1037e6a8e289ebdb8b43c0a6e 100644 (file)
--- a/src/cmd/ld/ldpe.c
+++ b/src/cmd/ld/ldpe.c
@@ -236,13 +236,6 @@ ldpe(Biobuf *f, char *pkg, int64 len, char *pn)
                s->p = sect->base;
                s->np = sect->size;
                s->size = sect->size;
-               if(s->type == STEXT) {
-                       if(etextp)
-                               etextp->next = s;
-                       else
-                               textp = s;
-                       etextp = s;
-               }
                sect->sym = s;
                if(strcmp(sect->name, ".rsrc") == 0)
                        setpersrc(sect->sym);
@@ -327,6 +320,12 @@ ldpe(Biobuf *f, char *pkg, int64 len, char *pn)
                        goto bad;
        
                s = sym->sym;
+               if(s->outer != S) {
+                       if(s->dupok)
+                               continue;
+                       diag("%s: duplicate symbol reference: %s in both %s and %s", pn, s->name, s->outer->name, sect->sym->name);
+                       errorexit();
+               }
                if(sym->sectnum == 0) {// extern
                        if(s->type == SDYNIMPORT)
                                s->plt = -2; // flag for dynimport in PE object files.
@@ -367,9 +366,27 @@ ldpe(Biobuf *f, char *pkg, int64 len, char *pn)
                        p->link = nil;
                        p->pc = pc++;
                        s->text = p;
-       
-                       etextp->next = s;
+               }
+       }
+
+       // Sort outer lists by address, adding to textp.
+       // This keeps textp in increasing address order.
+       for(i=0; i<obj->nsect; i++) {
+               s = obj->sect[i].sym;
+               if(s == S)
+                       continue;
+               if(s->sub)
+                       s->sub = listsort(s->sub, valuecmp, offsetof(Sym, sub));
+               if(s->type == STEXT) {
+                       if(etextp)
+                               etextp->next = s;
+                       else
+                               textp = s;
                        etextp = s;
+                       for(s = s->sub; s != S; s = s->sub) {
+                               etextp->next = s;
+                               etextp = s;
+                       }
                }
        }
 

diff --git a/src/cmd/ld/lib.h b/src/cmd/ld/lib.h
index d2a8b6fbefc745da5d955a2d146a3ebeff9ff400..acfad97751737740c166fdc9afcf619d6ae80e85 100644 (file)
--- a/src/cmd/ld/lib.h
+++ b/src/cmd/ld/lib.h
@@ -196,7 +196,6 @@ void        deadcode(void);
 Reloc* addrel(Sym*);
 void   codeblk(int32, int32);
 void   datblk(int32, int32);
-Sym*   datsort(Sym*);
 void   reloc(void);
 void   relocsym(Sym*);
 void   savedata(Sym*, Prog*, char*);
@@ -238,6 +237,8 @@ void        setpersrc(Sym*);
 void   doversion(void);
 void   usage(void);
 void   setinterp(char*);
+Sym*   listsort(Sym*, int(*cmp)(Sym*, Sym*), int);
+int    valuecmp(Sym*, Sym*);
 
 int    pathchar(void);
 void*  mal(uint32);

diff --git a/src/pkg/runtime/symtab.c b/src/pkg/runtime/symtab.c
index 24855868559a18adb1dfabc4591824da3bf43137..d7221c4767eda0fb92a9469a925d158f30e832b5 100644 (file)
--- a/src/pkg/runtime/symtab.c
+++ b/src/pkg/runtime/symtab.c
@@ -195,12 +195,13 @@ static int32 nfname;
 
 static uint32 funcinit;
 static Lock funclock;
+static uintptr lastvalue;
 
 static void
 dofunc(Sym *sym)
 {
        Func *f;
-
+       
        switch(sym->symtype) {
        case 't':
        case 'T':
@@ -208,6 +209,11 @@ dofunc(Sym *sym)
        case 'L':
                if(runtime·strcmp(sym->name, (byte*)"etext") == 0)
                        break;
+               if(sym->value < lastvalue) {
+                       runtime·printf("symbols out of order: %p before %p\n", lastvalue, sym->value);
+                       runtime·throw("malformed symbol table");
+               }
+               lastvalue = sym->value;
                if(func == nil) {
                        nfunc++;
                        break;
@@ -544,6 +550,7 @@ buildfuncs(void)
        // count funcs, fnames
        nfunc = 0;
        nfname = 0;
+       lastvalue = 0;
        walksymtab(dofunc);
 
        // Initialize tables.
@@ -553,6 +560,7 @@ buildfuncs(void)
        func[nfunc].entry = (uint64)etext;
        fname = runtime·mallocgc(nfname*sizeof fname[0], FlagNoPointers, 0, 1);
        nfunc = 0;
+       lastvalue = 0;
        walksymtab(dofunc);
 
        // split pc/ln table by func