Change ssl_EmulateSendFile to use NSPR 4.1 features

RESOLVED FIXED in 3.11.1

Status

NSS
Libraries
P2
enhancement
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: Wan-Teh Chang)

Tracking

3.11
3.11.1
All
Windows XP

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

21.49 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Julien Pierre
: review+
Details | Diff | Splinter Review
In bug 39011, regarding libSSL's new function ssl_EmulateSendFile, Wan-Teh wrote:

> NSPR 4.0 and earlier incorrectly assume that the mem mapping alignment 
> boundary is the page size.  
> To work around this NSPR bug, you have to make an OS-dependent call. 
> When you upgrade to NSPR 4.1 you will be able to call the new function
> PR_GetMemMapAlignment() instead. (But you might as well use the new 
> PR_EmulateSendFile function in NSPR 4.1 directly.)

I think NSS is now using NSPR 4.1.x, and so we should consider making the
changes Wan-Teh suggested.
(Reporter)

Updated

12 years ago
Summary: Chance ssl_EmulateSendFile to use NSPR 4.1 features → Change ssl_EmulateSendFile to use NSPR 4.1 features
(Assignee)

Comment 1

12 years ago
Created attachment 204598 [details] [diff] [review]
Proposed patch

This will eliminate all the code in lib/ssl/emulate.c.

See bug 34920 comment 2 on how PR_EmulateAcceptRead
and PR_EmulateSendFile should be used, and the transmitfile
method should be defined.

The only design issue is whether you want to have
PR_EmulateAcceptRead and PR_EmulateSendFile directly
in the ssl_methods structure, or you want to define
trivial ssl_AcceptRead and ssl_SendFile methods that
simply call PR_EmulateAcceptRead and PR_EmulateSendFile.
Let me know which you prefer.

I've verified that PR_EmulateAcceptRead and ssl_EmulateAcceptRead
are essentially the same.  I still need to compare
PR_EmulateSendFile with ssl_EmulateSendFile to make sure
they are essentially the same.  But please go ahead and
review this patch.
Attachment #204598 - Flags: review?(nelson)
(Assignee)

Comment 2

12 years ago
Created attachment 204601 [details] [diff] [review]
Alternate patch

This is the other design I described.
Attachment #204601 - Flags: review?(nelson)
(Reporter)

Comment 3

12 years ago
Comment on attachment 204598 [details] [diff] [review]
Proposed patch

I prefer this patch.  r=nelson
Attachment #204598 - Flags: review?(nelson) → review+
(Reporter)

Comment 4

12 years ago
Comment on attachment 204601 [details] [diff] [review]
Alternate patch

This patch is also acceptable, but I think the other patch is preferable.
Attachment #204601 - Flags: review?(nelson) → review+
(Assignee)

Comment 5

12 years ago
I checked in the proposed patch on the NSS trunk (3.12).

Removing emulate.c;
/cvsroot/mozilla/security/nss/lib/ssl/emulate.c,v  <--  emulate.c
new revision: delete; previous revision: 1.7
done
Checking in manifest.mn;
/cvsroot/mozilla/security/nss/lib/ssl/manifest.mn,v  <--  manifest.mn
new revision: 1.14; previous revision: 1.13
done
Checking in sslimpl.h;
/cvsroot/mozilla/security/nss/lib/ssl/sslimpl.h,v  <--  sslimpl.h
new revision: 1.44; previous revision: 1.43
done
Checking in sslsock.c;
/cvsroot/mozilla/security/nss/lib/ssl/sslsock.c,v  <--  sslsock.c
new revision: 1.46; previous revision: 1.45
done
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.12
(Assignee)

Updated

12 years ago
Attachment #204601 - Attachment is obsolete: true
(Reporter)

Comment 6

12 years ago
Comment on attachment 204598 [details] [diff] [review]
Proposed patch

I would like to eliminate any unnecessary libssl source code differences between the trunk and the 3.11 branch, so I am requesting second review on this patch for the 3.11 branch.
Attachment #204598 - Flags: review?(julien.pierre.bugs)
(Reporter)

Comment 7

12 years ago
reopening, so this will appear on our radar for 3.11.1
Status: RESOLVED → REOPENED
Priority: -- → P2
Resolution: FIXED → ---
Target Milestone: 3.12 → 3.11.1

Comment 8

11 years ago
Comment on attachment 204598 [details] [diff] [review]
Proposed patch

Looks fine. I didn't review the NSPR functions, only the NSS usage.
Attachment #204598 - Flags: review?(julien.pierre.bugs) → review+
(Reporter)

Comment 9

11 years ago
I back ported this fix to the NSS 3.11 branch.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.