txXPCOMExtensionFunction.cpp(190) : warning C4244: 'argument' : conversion from 'PRUnichar' to 'char', possible loss of data

RESOLVED FIXED in mozilla21

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Unassigned)

Tracking

(Blocks 1 bug)

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Reporter

Description

6 years ago
We hit this build warning on Windows, when building txXPCOMExtensionFunction.cpp:
{
e:/builds/moz2_slave/m-cen-w32-dbg/build/content/xslt/src/xpath/txXPCOMExtensionFunction.cpp(190) : warning C4244: 'argument' : conversion from 'PRUnichar' to 'char', possible loss of data
}
https://tbpl.mozilla.org/php/getParsedLog.php?id=19634130&tree=Firefox

The code:
> 183     PRUnichar letter;
[...]
> 190             methodName.Append(upperNext ? nsCRT::ToUpper(letter) : letter);

https://mxr.mozilla.org/mozilla-central/source/content/xslt/src/xpath/txXPCOMExtensionFunction.cpp?mark=183-183,190-190#181

As you can see there, |letter| is of type PRUnichar, whereas ToUpper() expects a char:

> 166   static char ToUpper(char aChar) { return NS_ToUpper(aChar); }
> 167   static char ToLower(char aChar) { return NS_ToLower(aChar); }
https://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsCRT.h?mark=166-167#166
Reporter

Comment 1

6 years ago
This code was added in bug 278981.

sicking actually called out this chunk as a possible-issue in bug 278981 comment 32:
> >+    while ((letter = *name)) {
> >+        if (letter == '-') {
> >+            upperNext = PR_TRUE;
> >+        }
> >+        else {
> >+            methodName.Append(upperNext ? nsCRT::ToUpper(letter) : letter);
> >+            upperNext = PR_FALSE;
> >+        }
> >+        ++name;
> >+    }
> 
> You should probably make sure that all characters in the name are ascii
> characters. If there is an UTF8 char in there you could end up calling
> ToUpper on the first byte in a multibyte character.

...but peterv says 2 comments later that  'letter' is going to be ASCII and hence in-bounds for a character, so it shouldn't be a problem:
> AFAIK (XP)IDL only allows ASCII for the method name.


So, if peterv is right on that, this bug probably just needs a bounds-checking assertion and a static-cast to char.
Depends on: 278981
OS: Linux → All
Hardware: x86_64 → All
Reporter

Comment 2

6 years ago
Posted patch fixSplinter Review
Attachment #712605 - Flags: review?(peterv)
Reporter

Updated

6 years ago
Blocks: 840189
Comment on attachment 712605 [details] [diff] [review]
fix

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

::: content/xslt/src/xpath/txXPCOMExtensionFunction.cpp
@@ +186,5 @@
>          if (letter == '-') {
>              upperNext = true;
>          }
>          else {
> +            NS_ASSERTION(nsCRT::IsAscii(letter),

Make it a MOZ_ASSERT.
Attachment #712605 - Flags: review?(peterv) → review+
Reporter

Comment 4

6 years ago
Switched to MOZ_ASSERT and pushed:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ae60b8d343e5
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/ae60b8d343e5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.