This is the mail archive of the cygwin-apps mailing list for the Cygwin project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [patch/rebase] Add a rebase database to keep track of DLL addresses


On 7/6/2011 2:30 PM, Corinna Vinschen wrote:
> On Jul  6 11:27, Charles Wilson wrote:
>> So, the "protection" is algorithmic -- you simply ensure that this
>> function is not used in cases where those pointers are null.
> 
> If the pointers can get NULL, it's a bug in the calling function.

Ack.

>> (And there ARE cases where they CAN be null: e.g. the newly-allocated
>> entries at the tail of the list -- since you allocate in rounded-up
>> chunks of 100(?); the name pointers are, of course, explicitly SET to
>> null when reading data in from the db, prior to reading and allocating
>> storage for the strings themselves, etc).
> 
> Oh, come on.

I didn't say there was anything wrong.  I'm trying to make sure *I*
understand: these functions do not check against null, but the
algorithm, elsewhere, is structured such that null checks are not
required.  That's fine; I just wanted to be sure I understood.

Look, I'm used to *extremely* defensive programming on critical embedded
systems, where a segfault means somebody [might] die. So every function
has to check and handle illegal input -- or has to document why it
doesn't need to do so; you don't treat segfaults as a
programmer-error-detection mechanism ("bug in the calling function").

rebase isn't that sort of program. I get that -- but it's still nice to
nail down exactly WHY this particular function doesn't need to validate
its input arguments.  You've explained that; thanks.

>> The main reasons for this rewrite are, that
>> 1. WinMe does not support the RebaseImage()
>> 2. the recent binutils strip command corrupts dll's linked with
>> debugging informations.
>>
>> OK, so #1 doesn't apply anymore (technically msys and MinGW still kinda
>> maybe support Win9x [*] in the "if you're lucky it'll work but don't
>> come crying to us if it doesn't").  I'm not really sure what the story
>> is behind #2...
> 
> This is very old stuff.

OK.

>> Anyway, now imagehelper uses cygwin pathname conversion functions and
>> the like, so it does a bit more than simply re-implementing ReBaseImage.
> 
> Right, but there wouldn't be a problem to do the path conversion before
> calling ReBaseImage64.

OK by me.  In fact, we'd probably want to avoid using imagehelper for
32bit, but ReBaseImage64 for 64bit; it would make sense to drop
imagehelper entirely and use ReBaseImage[64] throughout, right?

'Course, that's a whole different patch.

>>> Without this decision first, I didn't want to clutter the database
>>> file format with stuff it doesn't need.
>>
>> OK.
>>
>>> What's more, the rebase addresses of 32 and 64 bit DLLs have nothing to
>>> do with each other.  Since they never share the same VM space, they are
>>> disjunct.  I mulled over a combined database format for both versions,
>>> but it doesn't make sense.  I think, what we should do is to keep two
>>> database files, one for 32 and one for 64 bit DLLs, and handle them
>>> independently.  Change the magic number and you're all set.
>>
>> That sounds reasonable. 'Course, you're still talking about a single
>> *program* handling both, aren't you (maybe run twice, in two separate
> 
> Yes.
> 
>> 'modes'?)  Or would you actually need a *64bit* rebase.exe to handle the
>> 64bit dlls, and a 32bit one to handle the 32bit dlls?
> 
> No.  It's all about how to implement it.  There's no technical reason
> that a 32 bit app couldn't tweak a 64 bit DLL and vice versa.

OK.

> I think this goes over the top.  The patch adds new functionality
> without breaking the existing one.  Therefore, the new code won't run
> with additional tweaks on mingw, but it does *not* break mingw.

Semantics.  (NOTE: I am *not* suggesting a bunch of #ifdefs in usage()
and help() here...since we [I] will soon remedy the problem)  IMO,
advertising a particular feature via the man page or help output on
platform X, but not providing a working implementation OF that feature
on platform X -- is equivalent to introducing a bug on platform X,
because my working definition of [one sort of] bug is "creates more
questions I have to answer on some mailing list or other".

Whiny question on the mingw mailing list: 'Why doesn't rebase -foo
work?' == broken. 'Cause somebody @ mingw will have to either fix it, or
answer the question. Over and over. And over.  And "somebody" would
probably be me.

Now, in this case we [I] will soon ensure that the feature WILL work, so
the whiny question will never come up -- but as of the initial,
cygwin-only introduction...it won't quite work yet.  But the mingw
executable will /claim/ to have a nifty brand new feature.

>> What if a user's db gets scrogged -- how can you tell?  rebase -s -i
> 
> My dictionary doesn't know the word "scrogged", but I guess it's clear from
> the context :)

OMG. I just checked the urban dictionary and, um, let's just say that is
NOT what I meant.  =8-O

s/scrogged/corrupted/g

and the less said about that, the better.  I'm going to have to stop
using that word.

> If the db is broken, rebase overwrites it in the next run.  If the db is
> broken, somebody broke it externally.  The database format is so simple,
> it's quite tricky to break it from within rebase.
> 
>> Maybe the db impl should be in a separate source file, and then we could
>> have a separate, very dumb tool, that just dumps the contents. It'd be
>> packaged in a "rebase-devel" package and nobody (but you, me, and Jason,
>> and some user who we are trying to help) would ever install it...
> 
> Well, it's not much of a problem to use the -v flag as an indicator not
> to run the complicated preparation stuff in print_image_info.  If you
> insist, I'll add that. 

Nope, I don't insist.

> I don't think that debugging the file content is
> useful but it's fine with me.
> 
>> because what you're REALLY asking is
>> 	"Can I add new functionality for cygwin, that will make
>> 	 rebase.exe either non- or sub-functional (or worse,
>> 	 non-compilable) on msys and mingw" -- until somebody else
>> 	 makes a separate cleanup patch for those platforms?"
> 
> You're kidding, right?  I don't like the way how you imply that I try
> deliberately to break other targets.  That's the second time in this
> email.  As you saw in my previous patches I even created msys and mingw
> specific code.  The given functionality of rebase is not changed and
> runs still fine on msys and mingw.  I #ifdef'ed the Cygwin stuff for a
> reason and even this patch contains special #ifdefs for msys and mingw.
> I think I'm far from being amused :(

Dailing things back a little: in this patch you have done all of those
things you say, and I'm grateful.  My fear was that you were proposing,
going forward, to STOP doing even minimal accommodations for mingw/msys
and "concentrate on the Cygwin stuff" because you "don't care for
mingw".  If that fear was unfounded, then I'm sorry for stating otherwise.

It may have been a language issue: "don't care for X" implies "do not
like X", "think poorly of X", and similar -- as in "I don't care for
boiled beets" -- which puts a very negative light on the "concentrate on
the Cygwin stuff" statement.  Whereas "don't care *about* X" is probably
what you meant, and doesn't put such a negative light on the other
statement.

I repeat: I have no objections to this patch, as is.  But I'm not the
maintainer -- Jason is.

--
Chuck


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