Avoid unnecessary allocations in sdb_measureAccess

RESOLVED FIXED in 3.14.2

Status

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

3.14
3.14.2

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Created attachment 688474 [details] [diff] [review]
patch v1
Attachment #688474 - Flags: review?(rrelyea)
(Assignee)

Comment 2

6 years ago
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.
Blocks: 578561

Comment 3

6 years ago
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.
Attachment #688474 - Flags: review+
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
> 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.
(Assignee)

Comment 6

6 years ago
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()
(Assignee)

Comment 7

6 years ago
Created attachment 692631 [details] [diff] [review]
patch v2
Attachment #688474 - Attachment is obsolete: true
Attachment #688474 - Flags: review?(rrelyea)
Attachment #692631 - Flags: review?(wtc)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
Created attachment 693088 [details] [diff] [review]
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.
Attachment #692631 - Attachment is obsolete: true
Attachment #692631 - Flags: review?(wtc)
Attachment #693088 - Flags: review?(wtc)
(Assignee)

Comment 10

6 years ago
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.
Attachment #693088 - Flags: review?(wtc) → review?(rrelyea)
(Assignee)

Comment 11

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

Comment 12

6 years ago
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
Attachment #693088 - Flags: review?(rrelyea) → review+

Comment 13

6 years ago
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
(Assignee)

Comment 14

6 years ago
Created attachment 702877 [details] [diff] [review]
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
(Assignee)

Comment 15

6 years ago
Thanks to Bob and Wan-Teh for all the reviews, I apologize if I had been too impatient.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.