Closed Bug 283761 Opened 19 years ago Closed 19 years ago

selfserv use of memset is inefficient

Categories

(NSS :: Tools, defect, P2)

3.9.5
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: julien.pierre, Assigned: julien.pierre)

Details

Attachments

(1 file)

As part of the handleConnection function, selfserv performs a large memset (of
10KB) to zero a stack buffer, before invoking PR_Read. This is not really
necessary. All that's needed is to NULL-terminate the buffer after a read.
This particular memset is showing up as the most frequent CPU stack in
performance profiles, thus it somewhat invalidates any performance made with
selfserv.
Priority: -- → P2
Target Milestone: --- → 3.10
Attachment #175622 - Flags: review?(nelson)
Comment on attachment 175622 [details] [diff] [review]
remove memset and NULL terminate

r=nelson
I believe the code is as correct with this patch as without it.
Here is a suggestion for an additional improvement to the existing code.

>@@ -885,21 +884,23 @@
>     }
> 
>     while (1) {

Suggest you change that to 
      while (bufRem > 0) {

> 	newln = 0;
> 	reqLen     = 0;
>-	rv = PR_Read(ssl_sock, pBuf, bufRem);
>+	rv = PR_Read(ssl_sock, pBuf, bufRem - 1);
Attachment #175622 - Flags: review?(nelson) → review+
Jullien checked in the above patch.
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Comment on attachment 175622 [details] [diff] [review]
remove memset and NULL terminate

This patch is correct.	There is a subtle
difference from the old code because the
old code does not null terminate 'buf' if
the PR_Read call reads in 'bufRem' bytes
of data.  So the new code is more correct.

I agree that the sole purpose of the memset
call was to ensure that data in 'buf' was
null terminated, and our code depends on
that, because it passes 'buf' to string
functions such as PORT_Strstr and strncmp.
Attachment #175622 - Flags: superreview+
Re: Nelson's suggested change to the condition
for the while loop

In the new code, bufRem is always >= 1.  So

    while (bufRem > 0) {

is the same as

    while (1) {

The loop will terminate when bufRem becomes 1.
At that time, we will pass 0 as the third
argument to PR_Read, and PR_Read will return
either 0 or -1, both of which cause us to leave
the while loop.

We should avoid passing 0 as the third argument
to PR_Read, but I am not sure if we can simply
say

    while (bufRem > 1) {

because the proper action may be "goto cleanup".
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: