Replace PR_MIN with std::min in netwerk

RESOLVED FIXED in Firefox 51

Status

()

Core
Networking
P5
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cpeterson, Assigned: jj, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

unspecified
mozilla51
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [good first bug][lang=c++][necko-backlog])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
We want to replace NSPR's PR_MIN macro with the std::min() template function in Mozilla's C++ code. C code in the nsprpub/ and security/nss/ directories may use PR_MIN and should not be changed.

Here are the last two uses of PR_MIN in Mozilla's C++ code:

netwerk/sctp/datachannel/DataChannel.cpp
2271    size_t sendlen = PR_MIN(len, DATA_CHANNEL_MAX_BINARY_FRAGMENT);

netwerk/wifi/nsWifiAccessPoint.h
62  mSsidLen = PR_MIN(len, mozilla::ArrayLength(mSsid));

http://searchfox.org/mozilla-central/search?q=symbol:M_2e330213c06657be872ac554557742aa5667bd6c&redirect=false
(Reporter)

Updated

2 years ago
Keywords: good-first-bug
Putting this on the backlog. If someone doesn't pick it up as a first bug, we'll have to get to this eventually so we can get rid of PR_MIN.
Whiteboard: [good first bug][lang=c++] → [good first bug][lang=c++][necko-backlog]
Assignee: nobody → kylenosar

Updated

2 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Created attachment 8782918 [details] [diff] [review]
v1.diff
Attachment #8782918 - Flags: review?(cpeterson)
(Reporter)

Comment 3

2 years ago
Comment on attachment 8782918 [details] [diff] [review]
v1.diff

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

Thanks, Jinank! Your patch looks good. I just have some minor suggestions.

btw, it looks like this bug was already assigned to Kyle Nosar. It's usually a good idea to ask the assignee in the bug if they have already started working on a patch before submitting your own patch.

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +2268,4 @@
>      LOG(("Sending binary message length %u in chunks", len));
>      // XXX check flags for out-of-order, or force in-order for large binary messages
>      while (len > 0) {
> +      size_t sendlen = std::min(len,  (long unsigned int)DATA_CHANNEL_MAX_BINARY_FRAGMENT);

For this cast, please use size_t instead of long because len is type size_t in this function. Also, please use a C++ style cast like `static_cast<size_t>(DATA_CHANNEL_MAX_BINARY_FRAGMENT)`. It looks like there are two space character after the comma, but we should just use one.

::: netwerk/wifi/nsWifiAccessPoint.h
@@ +2,4 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#include <iostream>

Please #include <algorithm> for std::min instead of iostream. We should probably add #include <algorithm> to DataChannel.cpp, too (just above #include <stdio.h> at the top of that .cpp file).
Attachment #8782918 - Flags: review?(cpeterson) → feedback+
(Assignee)

Comment 4

2 years ago
Created attachment 8782960 [details] [diff] [review]
v2.diff

Sorry I just missed that it was already assigned. Fixed those bugs
Attachment #8782960 - Flags: review?(cpeterson)
Assignee: kylenosar → jinank94
Attachment #8782918 - Attachment is obsolete: true
(Reporter)

Comment 5

2 years ago
Thanks, Jinank. I pushed your patch to the Try test server:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e1f5fdf11d68
(Reporter)

Comment 6

2 years ago
Comment on attachment 8782960 [details] [diff] [review]
v2.diff

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

::: netwerk/sctp/datachannel/DataChannel.cpp
@@ +2269,4 @@
>      LOG(("Sending binary message length %u in chunks", len));
>      // XXX check flags for out-of-order, or force in-order for large binary messages
>      while (len > 0) {
> +      size_t sendlen = std::min(len, static_cast<size_t>(DATA_CHANNEL_MAX_BINARY_FRAGMENT));

On second thought, a shorter and cleaner fix than the static_cast<> is to specify a size_t template parameter for std::min like this:

  size_t sendlen = std::min<size_t>(len, DATA_CHANNEL_MAX_BINARY_FRAGMENT);

::: netwerk/wifi/nsWifiAccessPoint.h
@@ +60,4 @@
>  
>    void setSSIDRaw(const char* aSSID, unsigned long len) {
>      memcpy(mSsid, aSSID, mozilla::ArrayLength(mSsid));
> +    mSsidLen = std::min(len, mozilla::ArrayLength(mSsid));

oops. It looks like MSVC doesn't like this std::min() change in nsWifiAccessPoint.h:

> netwerk\wifi\nsWifiAccessPoint.h(64): error C2672: 'std::min': no matching overloaded function found

https://treeherder.mozilla.org/logviewer.html#?job_id=26056102&repo=try#L11788

Rather than casting len from unsigned long to size_t, I recommend changing the parameter type from `unsigned long len` to `size_t len`. size_t is a more correct type for a buffer length.

Another (pre-existing) problem with this function is that it can read past the end of the `aSSID` buffer if mozilla::ArrayLength(mSsid) is larger than aSSID's buffer size len! I suggest the following rewrite, which checks for the smaller buffer size (mSsid or aSSID) *before* copying aSSID to mSsid:

  void setSSIDRaw(const char* aSSID, size_t len) {
    mSsidLen = std::min(len, mozilla::ArrayLength(mSsid));
    memcpy(mSsid, aSSID, mSsidLen);
  }
Attachment #8782960 - Flags: review?(cpeterson) → feedback+
(Assignee)

Comment 7

2 years ago
Created attachment 8783157 [details] [diff] [review]
v3.diff
Attachment #8782960 - Attachment is obsolete: true
Attachment #8783157 - Flags: review?(cpeterson)
(Reporter)

Comment 8

2 years ago
Comment on attachment 8783157 [details] [diff] [review]
v3.diff

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

@ Jinank: Thanks! Your patch looks good to me. I started another Try server build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21b1df91f23f

@ Patrick, can you please review Jinank's patch? His patch:

1. Replaces two instances of NSPR's PR_MIN() with std::min() in netwerk/ code. These are the last two uses of PR_MIN() outside of nsprpub and nss.

2. Avoids a theoretical buffer over-read in setSSIDRaw(), though it probably never happened and would only affect Gonk:

http://searchfox.org/mozilla-central/search?q=setSSIDRaw
Attachment #8783157 - Flags: review?(mcmanus)
Attachment #8783157 - Flags: review?(cpeterson)
Attachment #8783157 - Flags: feedback+
Comment on attachment 8783157 [details] [diff] [review]
v3.diff

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

r+ on nsWifiAccessPoint.. but you'll need jesup for datachannel
Attachment #8783157 - Flags: review?(mcmanus)
Attachment #8783157 - Flags: review+
Attachment #8783157 - Flags: checkin?(rjesup)

Updated

2 years ago
Attachment #8783157 - Flags: checkin?(rjesup) → checkin+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
Comment on attachment 8783157 [details] [diff] [review]
v3.diff

I botched the flag asking checkin? instead of r?

you have r=mcmanus r=jesup
Attachment #8783157 - Flags: checkin+

Comment 11

2 years ago
Pushed by cpeterson@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b4d6ab6548d
Replace PR_MIN with std::min in netwerk. r=jesup,mcmanus
Keywords: checkin-needed
(Reporter)

Comment 12

2 years ago
Thanks for your contribution, Jinank! If you're looking for another bug to work on, check out the list of bugs on the Bugs Ahoy site:

http://www.joshmatthews.net/bugsahoy/?js=1&cpp=1&unowned=1
status-firefox51: --- → fixed
(Assignee)

Comment 13

2 years ago
(In reply to Chris Peterson [:cpeterson] from comment #12)
> Thanks for your contribution, Jinank! If you're looking for another bug to
> work on, check out the list of bugs on the Bugs Ahoy site:
> 
> http://www.joshmatthews.net/bugsahoy/?js=1&cpp=1&unowned=1

I am kind of working on 2 more bugs as soon as I complete them I would take up some new bugs. Thanks Chris!

Comment 14

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0b4d6ab6548d
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.