505553 - Implement Changes for System NSS

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

The design specified at https://wiki.mozilla.org/NSS_Shared_DB_And_LINUX notes that there would need to be some changes to NSS to handle translating multiple attempts to load the same PKCS #11 module with different parameters into opening new slots in the PKCS #11 module with the same parameters.

Softoken itself already defines a mechanism to do this under application control (called SECMOD_OpenUserDB()). This mechanism can be made general for other tokens which may wish to have similiar semantics. This make sense for for tokens that understand they way NSS allows instance specific parameters for the PKCS #11 module. Those that have a fixed set of slots (no matter how many times they are opened) will continue to function correctly by only loading the token once.

I've found a number of issues while experimenting with this, several are separable, so I'll write separate bugs for each and make them prereqs for this bug.

Comment 1

[Robert Relyea]

bug 51162 is the last separable bug for this feature. Once that is checked in, I'll attach a series of related patches to this bug. 

The patches will have the following functionality.
1) pk11load patch
   - allow us to explicitly specify another library as an 'internal' library.
   - allow multiple attempts to load the same library with different parameters. If the library is already loaded, use the NSS 'create new slot' protocol to create new slots on the existing library with the new parameters.
2) pk11pars patch
   - Add new code to break apart parameters to individual slot parameters, then recombine them in a new merged parameter set.
3) New Sample nsssysteminit library.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Bob, I think this set of changes probably ought to define NSS 3.13.

Comment 3

[Robert Relyea]

I'm OK with that, though I need it relatively soon for Red Hat as a patch to our existing 3.12.

bob

Comment 4

[Robert Relyea]

I also need this code as a base for additional changes (a bug I just wrote up).

Comment 5

[Robert Relyea]

Attachment: Combined patch

Comment 6

[Robert Relyea]

Attachment: new files for nss/cmd/sysinit

Comment 7

[Robert Relyea]

Attachment: loader changes.

These changes allow: 1) some 'internal' module to have some specified library location (instead of the implied softoken). 2) allow multiple loading of the some module. Additional load calls use the NSS 'open new slot' protocol to open additional slots in the already loaded module.

Comment 8

[Robert Relyea]

Attachment: Changes to pk11parse.

Crack open the argument list looking for strings to open new modules in. This only works for tokens that understand the token=<> which is part of the NSS new slot protocol.

Create a common 'double escape' function rather than continuing to duplicate it within NSS.

Comment 9

[Robert Relyea]

Attachment: rest of the patch.

updates to the callers of pk11load,

use the new double escape function and remove the old one.

fixes to handle the fact we may be calling stan functions before we have been fully initialized (adding new slots to the trust domain.... If called before we initialize, those are simply no-ops because the last step in initialization is to add all slots to the default trust domain),

add a new internal function to find a module based on the function pointer.

Comment 10

[Robert Relyea]

Comment on attachment 397999 [details] [diff] [review]
Combined patch

This is the combined patch. I've split the patch into digestible segments, but the whole patch will have to checked in at once.

Nelson, if you'd like to review this, feel welcome, I just don't want to overload you with reviews.

bob

Comment 11

[Robert Relyea]

Nelson, if you'd like to review this patch, feel welcome, I just don't want to
overload you with reviews.

(repeating because nelson was not yet on the CC list).

bob

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Bob, I'm always CC'ed on all NSS and NSPR bugs.  
I want to discuss the design with you before discussing the code.  
Even after re-reviewing the web page, I'm afraid that there are still 
some major points that aren't clear to me.  I'm willing to help try to 
improve the documentation if you're willing to help me better understand
the design.  Please call me.

Comment 13

[Wan-Teh Chang]

Comment on attachment 397999 [details] [diff] [review]
Combined patch

nss/cmd is the wrong place for the libnsssysinit library.
Libraries should be under nss/lib.

Comment 14

[Robert Relyea]

It's not really an NSS library.

libnsssysinit is really a sample, the library itself would be part of the OS.

bob

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 398001 [details] [diff] [review]
loader changes.

r=nelson after you fix one incorrect comment.

>+    /* first look for token= key words from the module spec */

The keyword is not token=, but rather tokens=  (plural).
This same error appears in other parts of this patch, too.

>+    /* module has been reloaded, this module itself is done, 
>+     * return to the caller */
>+    if (mod->functionList == NULL) {
>+	mod->loaded = PR_TRUE; /* technically the module is loaded.. */
>+	return SECSuccess;

In our review, you explained that setting mod->functionList to NULL 
in a module signified that this was a duplicate module entry, that
is about to be consolidated with the previous entry for the same
module.  Now, you know that and I know that, but it's still really
inobviously to other readers of this code.  The comments above don't
explain that at all.  It would be good to clarify those comments.

Comment 16

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 397999 [details] [diff] [review]
Combined patch

I'm canceling this review request for the big consolidated patch, because I'm going to review the individual component patches instead.

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 398000 [details] [diff] [review]
new files for nss/cmd/sysinit

These files do not build a command.  There is no main function.
They appear to build a library. So, IMO, this directory does not 
belong in nss/cmd, but in nss/lib.  If I am misunderstanding this
code, please clarify its intent for me.

I gather that you think of this as a "sample" code, or "framework" 
code rather than as actual code to be shipped with NSS 3.12.x.
If so, perhaps we need to do something in the makefiles to prevent 
the built library from being included in the shipped packages.  
But I don't think that's a reason to put it in cmd.

If there's some other reason to put it in cmd that I have failed to
understand, please spell that out for me.  

Comment on manifest.mn:
Please don't put the line
>+DEFINES = -DNSPR20
into new manifest.mn files.  It's an artifact of a time over 10 years ago
when we could build NSS with NSPR 1.0 or 2.0, but it's not needed any more.

The rest of my comments are bout 
>Index: security/nss/cmd/sysinit/nsssysinit.c

This file has two large separate ifdef blocks. 
The first has 3 alternative cases, which are:

>+#ifdef XP_UNIX
>+#ifdef XP_WIN
>+#else /* all others */

The second has two alternatives, which are

>+#ifdef XP_LINUX
>+#else /* all others */

It's not clear to me that the Unix and Linux cases want to be 
handled differently.  
Why wouldn't the Linux code be used on all Unix platforms? 
It's also not clear to me that Unix is defined whenever Linux 
is defined.  Is it?  
So, I'm suggesting that you change that to XP_UNIX.

Inside that 
>+#ifdef XP_LINUX
case, the function PRBool getFIPSMode() has an EMTPY definition.
It does nothing, not even returning any value.  That can't be right.
So, r- for that.


>+   PORT_Memcpy(nssdir, userdir, strlen(userdir)+1);

That's just PORT_strcpy(nssdir, userdir);
I don't think it's superior to the strcpy call in any way, 
so please use strcpy.

>+#else
>+#ifdef XP_WIN

Let's use 
#elif defined(XP_WIN)
here, instead.  That will eliminate one #endif.

>+char *getUserDB(void)
>+{
>+   /* use the registry to find the user's NSS_DIR. if no entry exists, creaate
>+    * one in the users Appdir location */
>+}
>+
>+char *getSystemDB(void)
>+{
>+   /* use the registry to find the system's NSS_DIR. if no entry exists, creaate
>+    * one based on the windows system data area */
>+}

These are also empty implementations.  I think that's just as bad as the empty
implementation of getFIPSMode.  Even if they just return strdups of constant 
strings, that's better than effectively returning a random value.

>+#else
>+#error "Need to write getUserDB and get SystemDB functions"
>+#endif
>+#endif
>+
>+#ifdef XP_LINUX
>+PRBool getFIPSMode()
>+{
>+}

There's the empty getFIPSMode.


>+    if (filename) PORT_Free(filename);
>+    if (stripped) PORT_Free(stripped);

Nit: Since PORT_Free checks its argument for NULL, the if test is unnecessary.

Comment 18

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 398003 [details] [diff] [review]
Changes to pk11parse.

In our discussion on Thursday, you explained that you are trying to 
establish a function naming style for this file, and also for the 
file pk11pars.h, with the following characteristics:

- all function names have the form prefix_suffix
- prefix is capitalized for functions exported from the shared lib
  and lower case for all others (private to the shared lib)
- first letter of suffix is lower case for static functions, and 
  upper case for all non-static functions.

That style differs from the existing NSS style in the following ways.
In the existing NSS style:

- static functions typically have no prefix, but they may have a 
  lower case prefix.
- all functions with prefix_suffix style names have a suffix that 
  starts with a capital letter. 

I'm really reluctant to allow yet another coding style into NSS. :(
But it seems that the files pk11pars.[ch] already use your new style,
to some extent, so I don't feel I can completely veto it, but I 
do want to ask that you use it completely consistently in these files.  
This patch does not follow it consistently.  For example, Functions
- secmod_doubleEscape
- secmod_mkTokenChild
- secmod_mkAppendTokensList
and perhaps others, are named in your new style for static functions,
but are not static functions.  Maybe the solution is to make them 
static, or maybe it is to change their names to follow the new 
convention for non-static function names.  But one of those changes
must be made.  I'd like to see the whole file be made consistent, at least 
for functions that are private and not exported from the shared lib.

In  security/nss/lib/pk11wrap/pk11pars.c

>+ * Find any token= values in the module spec. 
>+ * Always return a new spec which does not have any token= arguments.
>+ * If token= arguments are found, Split the the various tokens defined into

In all those comments above, the string "token=" is wrong.  
The correct string is "tokens=" (plural). 

>+ * an array of child specs to return.
>+ *
>+ * Caller is responsible for freeing the child spec and the new token
>+ * spec.
>+ */
>+
>+#define SECMOD_SPEC_COPY(new, start, end)    \
>+  if (end > start) {                         \
>+	int _cnt = end - start;	             \
>+	PORT_Memcpy(new, start, _cnt);       \
>+	new += _cnt;                         \
>+  }
>+
>+char *
>+secmod_ParseModuleSpecForTokens(char *moduleSpec, char ***children, 
>+				CK_SLOT_ID **ids)

The comment above is supposed to describe secmod_ParseModuleSpecForTokens
but because SECMOD_SPEC_COPY is defined first, the comment appears to 
describe it instead.  Please move the definition of SECMOD_SPEC_COPY so 
that it does not separate the comment from the function it describes.


>+void
>+secmod_FreeChildren(char **children, CK_SLOT_ID *ids)
>+{
>+    char **thisChild = children;
>+
>+    if (!children) {
>+	return;
>+    }
>+
>+    for (thisChild = children; thisChild && *thisChild; thisChild++ ) {
>+	PORT_Free(*thisChild);

Nit: The variable thisChild is initialized twice with the same value.

Finally, in 

>@@ -389,11 +773,19 @@ SECMOD_LoadModule(char *modulespec,SECMO
>     }
> 
>     /* load it */
>-    rv = SECMOD_LoadPKCS11Module(module);
>+    rv = secmod_LoadPKCS11Module(module, &oldModule);
>     if (rv != SECSuccess) {
> 	goto loser;
>     }
> 
>+    /* if we just reload an old module, no need to add it to any lists.
>+     * we simple release all our references */
>+    if (oldModule) {
>+	SECMOD_DestroyModule(module);
>+	SECMOD_DestroyModule(oldModule);
>+	return SECSuccess;
>+    }
>+

It's not obvious to me why it is correct to destroy BOTH references. 
I understand why you want to destroy the reference to module, because
module is the new duplicate reference to oldModule.  But destroying
oldModule seems to also destroy the old thing that it duplicated.
I think the objective is to go from 1 reference to 1 (not adding one)
but it seems that we go from 1 to zero.  Did a new reference to 
old Module get added somewhere?

Most of these problems are cosmetic, but there are enough of them that 
I think they add up to r-.

Comment 19

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 398008 [details] [diff] [review]
rest of the patch.

>Index: security/nss/lib/pk11wrap/secmodi.h

> extern SECMODModule *SECMOD_FindModuleByID(SECMODModuleID);
>+extern SECMODModule *secmod_findModuleByFuncPtr(void *funcPtr);

>+char *secmod_mkAppendTokensList(PRArenaPool *arena, char *origModuleSpec, 
>+				char *newModuleSpec, CK_SLOT_ID newID,
>+				char **children, CK_SLOT_ID *ids);
>+char *secmod_doubleEscape(const char *string, char quote1, char quote2);
>+
>+
>+

Bob,  You have a new function naming convention intended for static 
functions only, but you seem to be using it for all new functions. 
It appears in the names of new functions that are declared in header 
files, and functions declared in header files are NOT static
functions (by definition, almost).  That's my big problem with 
this patch.  But I have one other concern.

I still have sharp painful memories of all sorts of NSS crashes 
that occurred becuase of misbehaving PKCS#11 modules that led to 
inconsistent or incompletely structured NSS module, slot and token
structures.  I think we need to be more defensive about such 
possibilities.  So, in the new code below.

+    for(mlp = modules; mlp != NULL; mlp = mlp->next) {
+	if (funcPtr == mlp->module->functionList) {
+	    module = mlp->module;

I think it would be much safer (and lead to fewer crashes that we
must debug) if that if statement was instead coded as

+       if (mlp && mlp->module && 
+           funcPtr == mlp->module->functionList) {

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

er, make that

+       if (mlp->module && funcPtr == mlp->module->functionList) {

since we know that mlp is not NULL

Comment 21

[Robert Relyea]

RE comment 17.

> I gather that you think of this as a "sample" code, or "framework" 
> code rather than as actual code to be shipped with NSS 3.12.x.
> If so, perhaps we need to do something in the makefiles to prevent 
> the built library from being included in the shipped packages.  
> But I don't think that's a reason to put it in cmd.

This is the second comment I got on this issue. While I disagree, it's hard to argue that both nelson and wtc feel this is the wrong directory, so I'll move it in my next version of the patch.

> It's not clear to me that the Unix and Linux cases want to be 
> handled differently.  
> Why wouldn't the Linux code be used on all Unix platforms? 
> It's also not clear to me that Unix is defined whenever Linux 
> is defined.  Is it?  
> So, I'm suggesting that you change that to XP_UNIX.

It's hard for you to see, since the code was not in the patch. The relevant section is Linux only, not Unix in general. It uses the /proc filesystem to tell if we are in fips mode or not. 

bob

Comment 22

[Robert Relyea]

> I still have sharp painful memories of all sorts of NSS crashes 
> that occurred becuase of misbehaving PKCS#11 modules that led to 
> inconsistent or incompletely structured NSS module, slot and token
> structures. 
> 
> +    for(mlp = modules; mlp != NULL; mlp = mlp->next) {
> +    if (funcPtr == mlp->module->functionList) {
> +        module = mlp->module;
> 
> I think it would be much safer (and lead to fewer crashes that we
> must debug) if that if statement was instead coded as
> 
> +       if (mlp && mlp->module && 
> +           funcPtr == mlp->module->functionList) {

I can make the change, however it really is an assert if mlp->module is NULL. mlp's are Module List elements. Their only purpose is to hold a module (and a link list pointer). This is not the same as a partially created module or slot. If an mlp doesn't have a module, it doesn't exist:) I'd rather put an assert in secmod_AddModuleToList() if we are worried about this (though the PK11_ReferenceModule() call should handle that;).

If this assumption doesn't hold, PK11_GetAllTokens (which is usually the first thing any server calls) will crash pretty much immediately.

bob

Comment 23

[Robert Relyea]

> It's not obvious to me why it is correct to destroy BOTH references. 
> I understand why you want to destroy the reference to module, because
> module is the new duplicate reference to oldModule.

I'll update the comment.

> But destroying
> oldModule seems to also destroy the old thing that it duplicated.
> I think the objective is to go from 1 reference to 1 (not adding one)
> but it seems that we go from 1 to zero.  Did a new reference to 
> old Module get added somewhere?

Calls that return modules, return a referenced module to the caller. This prevents the object from 'diappearing' on the caller before they are through with it. Internally oldModule reference occurred when it was returned from the FindModuleByFuncPtr call. The reference was passed to the caller of load module, which now needs to be freed because we no longer have need of it.

bob

Comment 24

[Robert Relyea]

> I think the objective is to go from 1 reference to 1 (not adding one)
> but it seems that we go from 1 to zero.

No, we are going from a reference 2 to 1 on oldModule. Note 'module' and oldModule point to two different structures which represent the same underlying pkcs11 module. That's why 'module' needs to go away.

Comment 25

[Robert Relyea]

Attachment: v2:(not moved) Sample sysinit module from command. (for interdiff)

Comment 26

[Robert Relyea]

Attachment: v2: loader changes.

Comment 27

[Robert Relyea]

Attachment: V2: Changes to pk11parse.

Comment 28

[Robert Relyea]

Attachment: V2: rest of the patch.

Comment 29

[Robert Relyea]

Attachment: v2: new files for nss/lib/sysinit

Comment 30

[Robert Relyea]

Comment on attachment 402488 [details] [diff] [review]
v2: loader changes.

You've already given r+ (which some requested changes), but your other comments noted the name inconsistancies, so I fixed these as well.

Comment 31

[Robert Relyea]

Comment on attachment 402489 [details] [diff] [review]
V2: Changes to pk11parse.

address review comments

Comment 32

[Robert Relyea]

Comment on attachment 402490 [details] [diff] [review]
V2: rest of the patch.

address review comments.

Comment 33

[Robert Relyea]

Comment on attachment 402487 [details] [diff] [review]
v2:(not moved) Sample sysinit module from command. (for interdiff)

This is the wrong patch...

Comment 34

[Robert Relyea]

Attachment: v2-interdiff: (not moved) new files for nss/cmd/sysinit

Comment 35

[Robert Relyea]

Comment on attachment 402493 [details] [diff] [review]
v2: new files for nss/lib/sysinit

This is the patch for checkin. see v2-interdiff for an interdiff friendly version. The only differences between the two patches are the directory location.

Comment 36

[Wan-Teh Chang]

Bob, can we make a generic version of libnsssysinit.so that
each Linux distribution can customize with a text file?  It
will reduce the barrier of customizing libnsssysinit.so.

Comment 37

[Robert Relyea]

In a second rev, maybe. It should be relatively easy to customize the existing one, however. You simply change the get_list function. That being said, I'm not sure what another Linux distribution would change in this case. It's already following the proposed standard in the design. (perhaps other versions of Unix may have some changes, certainly windows and Mac would use it differently as well).

bob

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 402493 [details] [diff] [review]
v2: new files for nss/lib/sysinit

OK, we're getting very close here.  
Most of the rest of these issues are stylistic.
I expect you can turn this around in less than a day.

Issue 1: lack of consistency in function definitions.

There are stylistic inconsistencies throughout this patch 
with the way that functions are defined.  In some cases,
like the one below, the entire prototype appears on one line.

>+int testdir(char *dir)

In other cases, the function type appears on a line by itself
followed by the function name and its arguments on a second line. 

In come cases, the opening brace of the function appears on the
end of the line, like this example:

>+char *getSystemDB(void) {

In other cases, it appears on a line by itself, e.g. 

>+char *getUserDB(void)
>+{

Finally, in some cases (as above) functions that take no 
arguments are defined to take an explicit (void) argument,
but in other cases, as below, they are defined without any
arguments

>+static PRBool getFIPSEnv()
>+{

This file seems to display just about all combinations possible.
I believe the prevailing NSS style is:

- function type on one line
- function name and arguments one one or more separate lines
with explicit (void) where no arguments are used, and 
- opening brace on a line by itself.

Let's use that style consistently in this patch.

Issue 2:  Returning "0" or "1" for functions that are defined
to return PRBool.  

In functions that return PRBool, we see lines like:

>+	return 0;
>+	 return 1;

These should return PR_TRUE or PR_FALSE.
There are numerous functions with this issue.


Issue #3: really a curiosity:

>+static void
>+safestrcpy(char *target, char *src)
>+{
>+    while (*src) {
>+	*target++ = *src++;
>+    }
>+    *target = 0;
>+}

How is that any safer than normal strcpy?  
Is there a reason not to use libc's strcpy?

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 402488 [details] [diff] [review]
v2: loader changes.

r=nelson

Comment 40

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 402489 [details] [diff] [review]
V2: Changes to pk11parse.

r=nelson. thanks

Comment 41

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 402490 [details] [diff] [review]
V2: rest of the patch.

r=nelson

Comment 42

[Robert Relyea]

Attachment: v3: Sample sysinit module.

Comment 43

[Robert Relyea]

Comment on attachment 404063 [details] [diff] [review]
v3: Sample sysinit module.

Addressed issues 1 and 2 directly.

Addressed issue 3 with a better named function and a comment.

Comment 44

[Robert Relyea]

Attachment: Tinderbox catches a bug...

This patch Fixes a bug caught by Tinderbox Windows.

Windows noticed that we were returning a SECStatus for a SECModule *. I don't know why this wasn't caught on other platforms... probably because SECSuccess was '0' and it was treating this as a NULL return.

The patch is to return the oldModule rather than free it. The caller would free it once it was through poking at it.

This patch is checked in to cause tinder box to go green, but still needs a review. If the review is a full r- (that is the patch itself is fundamentally flawed, we can back out the full checking.

bob

Comment 45

[Robert Relyea]

Comment on attachment 404125 [details] [diff] [review]
Tinderbox catches a bug...

See my comment about why this change was made. This patch is currently checked in to make tb happy.

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 404125 [details] [diff] [review]
Tinderbox catches a bug...

Bob, I'm gonna let this go overnight and see what happens to the tinderboxes.  If they all go green, then r+.  If they all go some other color, then we'll revisit this question tomorrow (Friday).  Already two of the MinGW boxes have gone orange, reporting new leaks.  But I don't know if that's because of your patch or not.

Comment 47

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 404125 [details] [diff] [review]
Tinderbox catches a bug...

Tinderbox is green, so r=nelson

Comment 48

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 404063 [details] [diff] [review]
v3: Sample sysinit module.

Looking at a diff of this patch and the previous version, I see:

-static PRBool getFIPSEnv()
+static PRBool 
+getFIPSEnv()
            ^ needs void

 #ifdef XP_LINUX
 
-PRBool getFIPSMode()
+static PRBool 
+getFIPSMode()
             ^ needs void

 #else
-static PRBool getFIPSMode()
+static PRBool 
+getFIPSMode()
             ^ needs void


+ * According the strcpy man page:
+ *
+ * The strings  may  not overlap, and the destination string dest must be
+ * large enough to receive the copy.
+ * 
+ * This implementation allows src to overlap with target. 
+ */
 static void
-safestrcpy(char *target, char *src)
+overlapstrcpy(char *target, char *src)

OK, now I understand what you're trying to achieve.  The problem is that
this new implementation has the very same problem that libc's strcpy has.
It's no safer than libc's in the general case.  

Consider this snippet of code:

int testfunc(void)
{
    char foo[30] = { "abcdefg\n" };
           strcpy( foo + 4, foo );   // this will crash
    overlapstrcpy( foo + 4, foo );   // so will this!
    puts(foo);                       // not reached
}

Now, here's a really safe overlapping strcpy.
It won't crash in the above test.

char * 
strmove(char * dest, const char * src)
{
    if (src && dest) {
        size_t srclen = strlen(src);
        memmove(dest, src, srclen + 1);
    }
    return dest;
}

Comment 49

[Robert Relyea]

>        strcpy( foo + 4, foo );   // this will crash
>       overlapstrcpy( foo + 4, foo );   // so will this!

I only care about the case

overlapstrcpy(foo, foo+4);

The above will work under most strcpy commands, but I didn't feel it was guarrenteed.

bob

I'll fix the voids.

Comment 50

[Nelson Bolyard (seldom reads bugmail)]

(In reply to comment #49)
> I only care about the case
> overlapstrcpy(foo, foo+4);

Yes, but your new block comment says the function is safe for overlapping
copies, and doesn't qualify that by saying that it's safe only when the 
destination is at an address less than the address of the source.  

You could fix the comments ... or just use a function that really is safe,
such as the one I supplied.

Comment 51

[Robert Relyea]

> You could fix the comments..

Agreed. I thought I had a comment that made it clear it only supported overlap in one direction. If that's not the case I'll certainly fix that.

bob

Comment 52

[Robert Relyea]

Attachment: v4: Sample sysinit module.

Comment 53

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 405182 [details] [diff] [review]
v4: Sample sysinit module.

r=nelson

Comment 54

[Robert Relyea]

Comment on attachment 397999 [details] [diff] [review]
Combined patch

obsolte patch

Comment 55

[Kai Engert (:KaiE:)]

While above patch may be sufficient for the Mozilla client environment and hide the problem, it's still undesirable to have this binary file in the source tree.


Discussion 1:


If I understand correctly, directory lib/sysinit produces a simple static library file, which contains a single object file.

I don't understand why you copied the complicated config.mk file from lib/nss, which has a lot of rules for shared libraries etc. Wouldn't it have been sufficient to copy a much simpler file, for example the one from lib/pkcs7/config.mk ?


Discussion 2:

For some reason you want your library file to exclude the library prefix and suffix. Instead of libnsssysinit.a you want it to be nsssysinit.

I'm not trying to understand why you did, but I can see, in order to achieve your goal, you have added statement

  LIBRARY = nsssysinit

to file manifest.mn. According to my testing, this statement causes the library file to be produced in the source tree, while without this statement, it gets stored in the object directory.

I guess you shouldn't overwrite the usual complex rule for the LIBRARY filename, which I could find in coreconf/ruleset.mk, is usually 

  $(OBJDIR)/$(LIB_PREFIX)$(LIBRARY_NAME).$(LIB_SUFFIX)

If you really want to shorten the name, I propose you use

  LIBRARY = $(OBJDIR)/$(LIBRARY_NAME)

Comment 56

[Kai Engert (:KaiE:)]

Sorry, my previous comment 55 was orginally intended to refer to bug 546389 comment 3.

The current state of CVS builds binary file nsssysinit (which appears to be a library file despite the missing extension) inside the source tree, although it should be built in the object directory, that's what bug 546389 requests to change.

I've attached a "patch v2" to bug 546389 (attachment 427094 [details] [diff] [review]).

In that patch I propose to:
- stop building nsssysinit when building the Mozilla client
- revert lib/sysinit/config.mk to a very simple ruleset
- prefix the LIBRARY variable to include the OBJDIR

I'll leave it to you, whether you'd like this discussion in here, or in bug 546389.

Comment 57

[Wan-Teh Chang]

Bob, Kai: Let's mark this bug as fixed (in 3.12.5 or 3.12.6?) and
open new bugs to deal with its bugs.

Comment 58

[Robert Relyea]

Agreeded