818275 - Avoid unnecessary allocations in sdb_measureAccess

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Kai Engert (:KaiE:)]

I noticed that sdb_measureAccess will allocate a temporary string up to 10000 times.

I think allocation isn't necessary for testing, I propose that we use a single buffer for the whole loop.

Comment 1

[Kai Engert (:KaiE:)]

Attachment: patch v1

Comment 2

[Kai Engert (:KaiE:)]

I propose to treat this bug as a blocker for bug 578561, because:

Without this bug fixed, as a result of fixing bug 578561, some environments would suffer from this overhead for the first time.

Comment 3

[Wan-Teh Chang]

Comment on attachment 688474 [details] [diff] [review]
patch v1

Review of attachment 688474 [details] [diff] [review]:
-----------------------------------------------------------------

r=wtc. Thank you for the patch. I suggest some changes. Please
attach a new patch.

::: security/nss/lib/softoken/sdb.c
@@ +32,3 @@
>  #include "prsystem.h" /* for PR_GetDirectorySeparator() */
>  #include "sys/stat.h"
> +#include "sys/param.h" /* for MAXPATHLEN */

Remove this header (I described an alternative below).

Please take the opportunity to fix two other header #include's:

#include <stdio.h>
#include <sys/stat.h>

These are standard headers, so they should use <>.

@@ +348,5 @@
>      PRUint32 i;
>      PRIntervalTime time;
>      PRIntervalTime delta;
>      PRIntervalTime duration = PR_MillisecondsToInterval(33);
> +    char *path_buffer;

This file is not consistent in its variable naming convention.
I'd suggest naming this variable pathBuffer. You can avoid this
issue by using the original variable name |temp| or naming it
simply |path|.

@@ +355,5 @@
>      if (directory == NULL) {
>  	return 1;
>      }
>  
> +    path_buffer = PORT_Alloc(MAXPATHLEN);

It is best to avoid using MAXPATHLEN, which may not be portable.

You can calculate the necessary buffer size:
  strlen(directory) + 1 + 15 + 5 + 3 + 1

And verify that by checking the return value of PR_snprintf.

@@ -361,3 @@
>  	PRIntervalTime next;
>  
> -        temp  = sdb_BuildFileName(directory,"","._dOeSnotExist_", time+i, 0);

Please take the opportunity to remove the unused |flags| argument from
sdb_BuildFileName.

Comment 4

[Kai Engert (:KaiE:)]

I noticed the existing code has an additional bug,
it always uses the / character as the separator between directory and name,
but we should the platform specific one.

Comment 5

[Kai Engert (:KaiE:)]

> You can calculate the necessary buffer size:
>   strlen(directory) + 1 + 15 + 5 + 3 + 1

In your calculation you missed that that 5 isn't the maximum size of the variable number during the loop, because max isn't 10000, but "time + 10000".

This shows that we really need a safe calculation, if we don't want just a large buffer such as MAXPATHLEN.

Comment 6

[Kai Engert (:KaiE:)]

I think I found another bug, in sdb_BuildFileName,
which I'd also like to fix as part of this work.

sdb_BuildFileName will always use the / directory separator.
That should be replaced with PR_GetDirectorySeparator()

Comment 7

[Kai Engert (:KaiE:)]

Attachment: patch v2

Comment 8

[Wan-Teh Chang]

Comment on attachment 692631 [details] [diff] [review]
patch v2

Review of attachment 692631 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/softoken/sdb.c
@@ +339,5 @@
>  {
>      char *dbname = NULL;
>      /* build the full dbname */
> +    dbname = sqlite3_mprintf("%s%c%s%s%d.db",
> +			     directory, PR_GetDirectorySeparator(),

Add the (int)(unsigned char) typecast to the return value of
PR_GetDirectorySeparator():
    (int)(unsigned char)PR_GetDirectorySeparator()

PR_GetDirectorySeparator() returns 'char', but %c matches an int,
after conversion to 'unsigned char'.

Details: the (unsigned char) cast is necessary if the most significant
bit of the 'char' is 1. This is not the case for '/' and '\\', so the
(unsigned char) cast is for good coding style. Similarly, C automatically
promotes an unsigned char function argument to 'int', so the (int) cast
merely makes that explicit.

@@ +387,5 @@
> +
> +    strcpy(temp, directory);
> +    if (directory[directoryLength - 1] != PR_GetDirectorySeparator()) {
> +	temp[directoryLength++] = PR_GetDirectorySeparator();
> +    }

Nit: I think the caller should ensure that the directory pathname does not
end in a directory separator.

@@ +404,5 @@
> +	 * will be cut off from the constant part.
> +	 * This code assumes the directory name at the beginning of
> +	 * temp remains unchanged during our loop. */
> +        PR_snprintf(tempStartOfFilename, maxFileNameLen,
> +		    "%d%s", time+i, doesntExistName);

1. I suggest you also copy doesntExistName to |temp|. Then, the
PR_snprintf() call just needs to convert time+i to a string.

2. I suggest you use the %lu format to print time+i, and add a
(PRUint32) cast: (PRUint32)(time + i).

You can also just increment |time|: (PRUint32)(time++).

Note: PR_*printf functions use the %lu format for PRUint32. For the
*printf functions in libc, the %lu format is for "unsigned long", which
may be different from PRUint32.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v3

Attaching an updated patch that adds the suggested typecasts and uses %lu.


(In reply to Wan-Teh Chang from comment #8)
> Add the (int)(unsigned char) typecast to the return value of
> PR_GetDirectorySeparator():
>     (int)(unsigned char)PR_GetDirectorySeparator()

OK


> Nit: I think the caller should ensure that the directory pathname does not
> end in a directory separator.

But even if we fix it now, a future caller might do it wrong.

Instead of having all callers to the work to ensure that and still facing the risk of a bad caller, I prefer safe coding and ensuring our expectations it in the consuming code.


> 2. I suggest you use the %lu format to print time+i, and add a
> (PRUint32) cast: (PRUint32)(time + i).
> Note: PR_*printf functions use the %lu format for PRUint32. For the
> *printf functions in libc, the %lu format is for "unsigned long", which
> may be different from PRUint32.

OK


> 1. I suggest you also copy doesntExistName to |temp|. Then, the
> PR_snprintf() call just needs to convert time+i to a string.

But this kills the benefits of my trick to move the time to the beginning of the filename. The benefit is, in case the number ever gets longer than we had expected, snprintf would cut off text from the end of the string (from the constant part).


> You can also just increment |time|: (PRUint32)(time++).

But your suggestion breaks the logic of the loop, because the variable "time" is also used for delta time calculation.

Comment 10

[Kai Engert (:KaiE:)]

Comment on attachment 693088 [details] [diff] [review]
Patch v3

It seems that Wan-Teh has lost interested in pushing this forward. I think my arguments in comment 9 have been reasonable, and explain why it makes sense to not implement some of Wan-Teh's change proposals. I don't understand why this work has stalled afterwards. I'll ask Bob to review the patch instead.

Comment 11

[Kai Engert (:KaiE:)]

This patch together with the patch from bug 578561 has passed the NSS test suite on all platforms in a "try build".

Comment 12

[Robert Relyea]

Comment on attachment 693088 [details] [diff] [review]
Patch v3

r+

But please make one change.
When you moved the non-unique part of the filename to the end, you ended up loosing the leading '.', which made the file 'hidden' to normal ls. You should add it back and remove it from the trailer... so  
   const char *doesntExistName = "._dOeSnotExist_.db";
becomes
   const char *doesntExistName = "_dOeSnotExist_.db";

and
       "%lu%s", (PRUint32)(time+i), doesntExistName);
becomes
       ".%lu%s", (PRUint32)(time+i), doesntExistName);

bob

Comment 13

[Robert Relyea]

re comment 10.

I suspect it's timing. Your previous comment was Dec 17. Lots of things fall on the floor around Christmas time. (I suspect you could find the pattern in a number of NSS bugs).

bob

Comment 14

[Kai Engert (:KaiE:)]

Attachment: patch v3 with tweak - as checked in

Checking in sdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/sdb.c,v  <--  sdb.c
new revision: 1.29; previous revision: 1.28
done

Comment 15

[Kai Engert (:KaiE:)]

Thanks to Bob and Wan-Teh for all the reviews, I apologize if I had been too impatient.