This is the mail archive of the mailing list for the binutils project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]

Re: Serious bug in auto-import

Paul Sokolovsky wrote:

> CW> Please take a look at the thread "[ A serious
> CW> bug of "ld --enable-auto-import"]" starting here:
> CW>
> CW> This is a confirmed bug.  I've tried to analyze it but I can't really
> CW> come up with a fix; I wonder if a fix is possible at all.  If this
> CW> problem can't be fixed, I'm going to have to recommend removing the
> CW> auto-import stuff. (Yes, it's that serious.)
> CW> PLEASE take a look and at least let me know you've seen it; it was
> CW> originally reported early Friday morning to the cygwin-apps list, and
> CW> moved to the binutils list on Saturday.
>     I'm sorry, I was on vacation abroad.

Oh -- this bug cropped up at a "bad" time for you. :-)   Thanks for 

>     Yes, the weird stuff. Of course, I though about the issue when
> started to work on auto-import, but my reasoning was misgone along the
> lines which were evident in this thread too: I confused *relocs* and
> *import thunks*. The former support addends, the latter - no. Well, I
> never bothered to check issue afterwards.
>      How disabling such a misfeature can be? Well, my opinion is that
> sample code far-fetched and presents, err, bad software design. The
> only sane usage of accessing external array with constant indexes I
> can imagine is something like
>           extern double world_transform_matrix[3][3];
>           ...
>      I doubt much code uses such a horror, so losing ability to auto-import
> contant-offset array for me is not a big loss. 

Agree.  (NOTE: following paragraph was written before I read the rest of 
your message.  You've already implemented the following! :-)

But, if code DOES use this (assuming we can't "fix" it) -- then the poor 
unsuspecting user should be warned "Attempt to access array via constant 
offset" or some such, with a pointer to some documentation somewhere 
that says "The following idiom is not supported in client code for array 
variables exported from DLLs on pei-386:
    "array_exported_from_dll[constant_index] = foo"
or "foo = array_exported_from_dll[constant_index]"
Please replace with the following idiom:
    <type> *local_copy = array_exported_from_dll;
    local_copy[constant_index] = foo;
or foo = local_copy[constant_index];"
(yeah, yeah, I know.  pointers, arrays.  the above is meant as a 
starting point; not to be taken literally)

> Much more pitiful fact
> is that it also applies to accessing global extern struct's - I bet
> periodically code which does that will be hit. 

Oooh.  I hadn't thought of that.

> Well, nothing comes for
> free.
>      Actions to be taken: well, I don't think it's productive idea,
> having hit some particular difficulty, give up entirely thing which
> otherwise works well.

True.  I panicked.  (At least it was private mail; I hadn't (and still 
have not)  made that suggestion onlist.

> Imho, the better idea is to make quick surgery
> to localize the problem. That's exactly what attached patch does:
> ====
> #include <stdio.h>
> extern struct
> {
>   int a;
>   int b;
> } st;
> int main(void){
>         printf("%d\n", st.b);
>         printf("%d\n", st.a);
> }
> ====
> gcc -s -Wl,--enable-auto-import -o hello.exe hello.o -L. -lhwstr
> Warning: resolving _st by linking to __imp__st (auto-import)
> hello.o(.text+0x13):hello.c: aggregate 'st' is referenced in direct addressing m
> ode with constant offset - auto-import failed. Workarounds: a) mark the symbol w
> ith __declspec(dllimport); b) use auxiliary variable
> make: *** [hello.exe] Hangup

AHA!  Exactly what I suggested above -- but I didn't know how to make ld 
do that.  Very cool.  Good work, Paul.  Warning when something fails and 
identifying the spot where the failure occurs is MUCH better than 
silently allowing buggy code. :-)

I'm going to test this; assuming it works as advertised (and I have no 
reason to think it will not) I recommend immediate inclusion.

THEN we can think about a "real" fix for this array-index problem (if 
possible; it may not be). :-)


P.S. Paul: also, please take a look here:
ld-auto-import memory bug fixing

Ralf has been trying (without success) to track down a memory leak 
problem that makes it difficult (impossible?) to link KDE apps to KDE 
DLL's via auto-import.  Too many dll's, too many symbols + memory leak 
== out of memory. The memory leak *may* be related to this snippet of 
code in pe-dll.c(pe_walk_relocs_of_symbol):

+ /* Warning: the allocated symbols are remembered in BFD and reused
      later, so don't free them! */
!!! Where do they be used later ??? Can this not be done in another way ?
   /* free (symbols); */

And here, in pe.em(pe_find_data_imports)

+            int nsyms, symsize, i;
+            symsize = bfd_get_symtab_upper_bound (b);
symbols will never be freed, only overwritten
+            symbols = (asymbol **) xmalloc (symsize);

Original bug description is here:
WG: ld --auto-import for cygwin and libtool

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]