853159 - merge hex decoding functions throughout mailnews

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[:aceman]

Merge the various hex decoding functions (*unhex) throughout mailnews as the comment in mailnews/local/src/nsParseMailbox.h suggests.

Comment 1

[:aceman]

Attachment: patch

Comment 2

[Joshua Cranmer [:jcranmer]]

Comment on attachment 727383 [details] [diff] [review]
patch

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

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +1097,5 @@
>        PR_ASSERT(*s != ' ' && *s != '\t');
>        m_flags = 0;
> +      for (i = 0; i < 4; i++) {
> +        uint8_t unhex = msg_UnHex(*s);
> +        if (unhex == msg_HexInvalid)

I just want to point out, as a drive-by, that this if statement will never be true. C++ specifies that both operands get uplifted to an int (in this case) before doing the comparison. So int8_t of -1 becomes uint8_t of 255 becomes int 255, which is clearly not int -1.

Comment 3

[:aceman]

Yeah, thanks. It should probably be int8_t as in the other occurrences.

Comment 4

[:aceman]

Attachment: patch v2

Do you think you could review this?

Comment 5

[Joshua Cranmer [:jcranmer]]

Comment on attachment 750706 [details] [diff] [review]
patch v2

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

In general, the string classes have a method:
int32_t ToInteger( nsresult* aErrorCode, uint32_t aRadix=10 ) const;

By passing aRadix as 16, you can decode a hex string. I think most of these conversions can be handled like that.

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ -2280,5 @@
>  
> -        int8_t unhex = ((C >= '0' && C <= '9') ? C - '0' :
> -            ((C >= 'A' && C <= 'F') ? C - 'A' + 10 :
> -             ((C >= 'a' && C <= 'f') ? C - 'a' + 10 : -1)));
> -        if (unhex < 0)

For example, you could probably replace this entire loop with:

nsresult rv;
result = nsDependentCString(p, numChars).ToInteger(&rv, 16);
if (NS_FAILED(rv))
  result = 0;

Comment 6

[:aceman]

If you look at the whole loop, the numChars is only a max number of characters to look at. If a nonhex char is encountered sooner the loop is exited but the accumulated result is kept. Is that what nsDependentCString(p, numChars).ToInteger() does too? I am not sure.

   for (i=0, result = 0; i < numChars; i++, p++)
    {
        char C = *p;

        int8_t unhex = msg_UnHex(C);
        if (unhex == msg_HexInvalid)
            break;
        result = (result << 4) | unhex;
    }

Also, I'd imagine nsDependentCString().ToInteger() to be orders of magnitude slower.

Comment 7

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #6)
> If you look at the whole loop, the numChars is only a max number of
> characters to look at. If a nonhex char is encountered sooner the loop is
> exited but the accumulated result is kept. Is that what
> nsDependentCString(p, numChars).ToInteger() does too? I am not sure.

That is indeed what is done. Note that in most cases, this would be an error situation that isn't worth worrying about since it means we have serious file corruption.

> Also, I'd imagine nsDependentCString().ToInteger() to be orders of magnitude
> slower.

The only code likely to be called enough for something to make a difference is nsMsgFolderCacheElement, and from several performance experiments I've done in the past, the most performance-intensive part of this function is the "return NS_ERROR_FAILURE" a few lines earlier (throwing JS exceptions is very expensive).

Comment 8

[:aceman]

(In reply to Joshua Cranmer [:jcranmer] from comment #7)
> (In reply to :aceman from comment #6)
> > If you look at the whole loop, the numChars is only a max number of
> > characters to look at. If a nonhex char is encountered sooner the loop is
> > exited but the accumulated result is kept. Is that what
> > nsDependentCString(p, numChars).ToInteger() does too? I am not sure.
> 
> That is indeed what is done. Note that in most cases, this would be an error
> situation that isn't worth worrying about since it means we have serious
> file corruption.

I didn't understand this. Anyway, we would need both versions. Code looping only while digits are found then then returning the value. And code looping over a fixed number of characters and if any one of them is invalid (non-hex) the whole number is invalid.

Comment 9

[[e-mail]]

(In reply to aceman from comment #8)
> (In reply to Joshua Cranmer from comment #7)
> > Note that in most cases, this would be an error situation that isn't
> > worth worrying about since it means we have serious file corruption.
> 
> I didn't understand this.

This particular value was previously converted to hex before being written to the file. Only some sort of corruption could change it into an error condition.

Comment 10

[[e-mail]]

Comment on attachment 750706 [details] [diff] [review]
patch v2

I would separate out the invalid hex detection (using isxdigit from ctypes would appear to be the way to go there) from the hex conversion (which would return zero for an invalid digit).

Comment 11

[:aceman]

So after converting the function to nsCString(char*, length).ToInteger(&rv, radix16) I found out it only returns int32_t (it fails on "ffffffff" so we return 0, which is bad). We need a version for at least uint32_t and also for uint64_t (in nsMsgDatabase::YarnToUInt64). Any further ideas?

Comment 12

[[e-mail]]

I suppose you could add FromHex(char*, int) and FromHex64(char*, int) functions to nsMsgUtils.cpp (although MsgStripQuotedPrintable would still have to do its own checking).

Comment 13

[:aceman]

I do not understand. How would the functions operate internally? You guys wanted nsCString.ToInteger() but that seems unusable. So is there a different implementation than going back to the original approach (but still taking multiple char strings) ?

Comment 14

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #11)
> So after converting the function to nsCString(char*, length).ToInteger(&rv,
> radix16) I found out it only returns int32_t (it fails on "ffffffff" so we
> return 0, which is bad). We need a version for at least uint32_t and also
> for uint64_t (in nsMsgDatabase::YarnToUInt64). Any further ideas?

Such as nsCString::ToInteger64?

Comment 15

[:aceman]

Thanks, that is better. That one is missing from https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Glue_classes/nsCString .
Still, int64_t is not uint64_t but I can probably cope.

And there are still cases where we compute an uint32_t and then convert it to int32_t. I wonder if it changes the semantics of the original code (whether it copes differently with >2^31).

Comment 16

[:aceman]

Attachment: patch v3

So how do you like this one?

Comment 17

[[e-mail]]

Comment on attachment 8417024 [details] [diff] [review]
patch v3

>+  NS_WARN_IF_FALSE(result <= LONG_MAX, "UnHex result higher than int32_t!");
The corresponding SetInt32Property uses the %x format string to generate the hex. However, %x is an unsigned format, so for example -1 will become FFFFFFFF.

>       // decode the second quoted char
>       c = (c << 4);
>-      if (token[2] >= '0' && token[2] <= '9')
>-        c += token[2] - '0';
>-      else if (token[2] >= 'A' && token[2] <= 'F')
>-        c += token[2] - ('A' - 10);
>-      else if (token[2] >= 'a' && token[2] <= 'f')
>-        c += token[2] - ('a' - 10);
>+      if (isxdigit(token[2])) {
>+        c += msg_UnHex(token + 2, 1);
>+      }
[Perhaps you could use msg_UnHex(token + 1, 2) once that you've verified that you do in fact have two hex digits.]

>+inline
[Not sure whether this should be inline.]
I had a look at nsStringAPI.h and it just uses PR_sscanf("%x"), which calls through to strtoul, meaning that the external API returns -1 (instead of failure) for NS_LITERAL_STRING("FFFFFFFF").ToInteger(16), so perhaps we could use that (or strtoull) directly.

Comment 18

[:aceman]

(In reply to [e-mail] from comment #17)
> >+  NS_WARN_IF_FALSE(result <= LONG_MAX, "UnHex result higher than int32_t!");
> The corresponding SetInt32Property uses the %x format string to generate the
> hex. However, %x is an unsigned format, so for example -1 will become
> FFFFFFFF.

I only see:
NS_IMETHODIMP nsMsgFolderCacheElement::SetInt32Property(const char *propertyName, int32_t propertyValue)
{
  nsAutoCString propertyStr;
  propertyStr.AppendInt(propertyValue, 16);
  ...
}

So where is the %x?

> >+inline
> [Not sure whether this should be inline.]
It must be inline otherwise I get "multiple definition of 'msg_IsHex()'" link errors. All other functions in that file are inline so that looks like a rule there. Probably because it is a .h file.

> I had a look at nsStringAPI.h and it just uses PR_sscanf("%x"), which calls
> through to strtoul, meaning that the external API returns -1 (instead of
> failure) for NS_LITERAL_STRING("FFFFFFFF").ToInteger(16), so perhaps we
> could use that (or strtoull) directly.
But I think we do not want to return -1 for "FFFFFFFF" at least for proper uint32_t callers.
Also %x pre the docs seems to accept 0x and + and - prefixes, which we do not want either.

Comment 19

[[e-mail]]

(In reply to aceman from comment #18)
> (In reply to comment #17)
> > >+  NS_WARN_IF_FALSE(result <= LONG_MAX, "UnHex result higher than int32_t!");
> > The corresponding SetInt32Property uses the %x format string to generate the
> > hex. However, %x is an unsigned format, so for example -1 will become
> > FFFFFFFF.
> 
> I only see:
> NS_IMETHODIMP nsMsgFolderCacheElement::SetInt32Property(const char
> *propertyName, int32_t propertyValue)
> {
>   nsAutoCString propertyStr;
>   propertyStr.AppendInt(propertyValue, 16);
>   ...
> }
> 
> So where is the %x?

In the definition of AppendInt, if you pass 16 as the base, it uses the %x format.

> > >+inline
> > [Not sure whether this should be inline.]
> It must be inline otherwise I get "multiple definition of 'msg_IsHex()'"
> link errors. All other functions in that file are inline so that looks like
> a rule there. Probably because it is a .h file.
Indeed, but what I meant was that the function body should be moved to the cpp file.

> > I had a look at nsStringAPI.h and it just uses PR_sscanf("%x"), which calls
> > through to strtoul, meaning that the external API returns -1 (instead of
> > failure) for NS_LITERAL_STRING("FFFFFFFF").ToInteger(16), so perhaps we
> > could use that (or strtoull) directly.
> But I think we do not want to return -1 for "FFFFFFFF" at least for proper
> uint32_t callers.
> Also %x pre the docs seems to accept 0x and + and - prefixes, which we do
> not want either.

So does ToInteger. The only difference is that the internal API version limits* you to 0x7FFFFFFF (so it fails to round-trip -2^31 for instance).

*It bails out if any intermediate prefix results in a lower number but this is done in 32-bit arithmetic so it actually allows a large number of invalid** numbers such as 10000000000. (When autodetecting hexadecimal it assumes decimal until it finds a hexadecimal digit. It's possible that it rejects a decimal number as being out of range before finding a hexadecimal digit when it would not have rejected the same number for being hexadecimal.)

**When using the latest clang compiler optimisations integer overflow might be assumed not to occur and so it wouild actually accept any number.

Comment 20

[:aceman]

(In reply to [e-mail] from comment #19)
> > But I think we do not want to return -1 for "FFFFFFFF" at least for proper
> > uint32_t callers.
> > Also %x pre the docs seems to accept 0x and + and - prefixes, which we do
> > not want either.
> 
> So does ToInteger. The only difference is that the internal API version
> limits* you to 0x7FFFFFFF (so it fails to round-trip -2^31 for instance).

But we do not want to support that. No 0x prefixes, only the pure hex digits.

So what can we do now? Run the string through msg_IsHex and only then pass to nsCString.ToInteger64()?
In the current patch I have numbers>0x7FFFFFFF solved (and also <=0xFFFFFFFFFFFFFFFF). But it is an ugly hack so I still think 'patch v1+expansion to process strings' was faster and cleaner.

Comment 21

[[e-mail]]

(In reply to aceman from comment #20)
> So what can we do now? Run the string through msg_IsHex and only then pass
> to nsCString.ToInteger64()?
Well, ToInteger wasn't my suggestion, so I don't know what to say there, but I would be happy with a central loop but of the form that you had in attachment 750706 [details] [diff] [review].

Comment 22

[:aceman]

(In reply to [e-mail] from comment #21)
> (In reply to aceman from comment #20)
> > So what can we do now? Run the string through msg_IsHex and only then pass
> > to nsCString.ToInteger64()?
> Well, ToInteger wasn't my suggestion, so I don't know what to say there, but
> I would be happy with a central loop but of the form that you had in
> attachment 750706 [details] [diff] [review].

OK. So we need jcranmer's opinion now.

Comment 23

[Joshua Cranmer [:jcranmer]]

If ToInteger is proving to be such a pain, perhaps you shouldn't use it.

But it also may be worth filing a bug on getting a string-to-unsigned-integer conversion for our string classes.

Comment 24

[:aceman]

The >2^63 values can be worked around (as in the patch), but I worry that it silently accepts also strings with 0x or + and - prefixes (and who knows what else may be added), which we definitely do not want (at least the callers I convert do not expect them).

So instead of tons of checks and hacks with ToInteger, I would rather use a plain readable loop checking each character. As you said speed is not a concern (even though I think it will be faster).

Comment 25

[:aceman]

Attachment: patch v4

So this is back to iterating over hex chars, however it takes the whole string to convert. So the removal of loops from callers can still be done.

Comment 26

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8421174 [details] [diff] [review]
patch v4

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

I didn't look at this in too gory detail, I'll assume that Neil is going to do that. But I do have a lot of little nits and comments:

::: mailnews/base/src/nsMsgFolderCacheElement.cpp
@@ +67,5 @@
>    if (resultStr.IsEmpty())
>      return NS_ERROR_FAILURE;
>  
> +  uint32_t result = msg_UnHex(resultStr.get(), resultStr.Length());
> +  NS_WARN_IF_FALSE(result <= LONG_MAX, "UnHex result higher than int32_t!");

uint64_t result = ...
NS_WARN_IF_FALSE(result <= INT32_MAX, ...)
*aResult = (int32_t)result;

[we're storing as a signed 32-bit integer and reading as an unsigned 32-bit integer? that can't go over well...]

::: mailnews/base/util/nsMsgUtils.h
@@ +538,5 @@
> + * Processes up to aNumChars characters or the first non-hex char.
> + * It is not an error if less than aNumChars valid hex digits are found.
> + */
> +inline
> +uint64_t msg_UnHex(const char *aHexString, uint8_t aNumChars) {

So the purist in me wants to place these methods in a mozilla::mailnews namespace, although I think I'm in the minority of wanting to use namespaces in comm-central.

In any case msg_UnHex violates style guidelines for naming.

And I think we're still stuck on brace-per-line style in C++ code?

@@ +539,5 @@
> + * It is not an error if less than aNumChars valid hex digits are found.
> + */
> +inline
> +uint64_t msg_UnHex(const char *aHexString, uint8_t aNumChars) {
> +  // Large numbers will not fit into uint64_t .

Nit: superfluous space.

@@ +545,5 @@
> +
> +  uint64_t result = 0;
> +  for (uint8_t i = 0; i < aNumChars; i++) {
> +    result <<= 4;
> +    unsigned char C = aHexString[i];

Nit: lowercase C here.

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +1094,3 @@
>        char *s = value;
>        PR_ASSERT(*s != ' ' && *s != '\t');
> +      NS_ASSERTION(msg_IsHex(s, 4), "Expected 4 hex digits for flags.");

Sigh. Will we ever standardize on a single assertion in our code? All we need is a MOZ_ASSERT line after this to round it out.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +3877,5 @@
>  // WARNING - if yarn is empty, *pResult will not be changed!!!!
>  // this is so we can leave default values as they were.
>  /* static */void nsMsgDatabase::YarnToUInt32(struct mdbYarn *yarn, uint32_t *pResult)
>  {
> +  uint8_t numChars = std::min((mdb_fill)8, yarn->mYarn_Fill);

hmm.
Does uint8_t numChars = std::min<uint8_t>(8, yarn->mYarn_Fill); compile? that seems like it would be better to me.

Comment 27

[:aceman]

Comment on attachment 8421174 [details] [diff] [review]
patch v4

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

::: mailnews/base/util/nsMsgUtils.h
@@ +545,5 @@
> +
> +  uint64_t result = 0;
> +  for (uint8_t i = 0; i < aNumChars; i++) {
> +    result <<= 4;
> +    unsigned char C = aHexString[i];

This produces wrong results if there are less than aNumChars hex digits. I'll fix it but you can still focus on the rest of the patch that will not change for that.

Comment 28

[:aceman]

Attachment: patch v5

Thanks.
I reworked the nsMsgFolderCacheElement.cpp::GetInt32Property() part to do decoding with sscanf("%x") instead of unhex, because I found out we really do encode a value of -1 in SetInt32Property and it converts to "ffffffff". So as Neil pointed out, AppendInt uses the "%x" format string internally to encode the number, then I let the same format string decode the string again. So we do not need to hope int32_t(uint32_t) works, we let "%x" handle the conversions.

Comment 29

[[e-mail]]

Comment on attachment 8422768 [details] [diff] [review]
patch v5

>+  int32_t result;
[Why not sscanf directly into aResult?]

>+  if (PR_sscanf(resultStr.get(), "%x", &result) == 1)
Isn't 1 the "success" value?

>+      if (MsgIsHex(token + 1, 2)) {
>+        // If we got here, we successfully decoded a quoted printable sequence,
>+        // so bump each pointer past it and move on to the next char.
>+        dest[destIdx++] = MsgUnhex(token + 1, 2);
[Given that there are hardly any uses of token left, you might as well just use src + srcIdx + 1 directly.]

>+inline
>+uint64_t MsgUnhex(const char *aHexString, uint8_t aNumChars)
[I still think the definition of this belongs in nsMsgUtils.cpp]

>+  // Large numbers will not fit into uint64_t.
>+  NS_ASSERTION(aNumChars <= 16, "Hex literal too long to convert!");
[As you only pass 8 maximum, why bother with uint64_t ?]

>+inline
>+bool MsgIsHex(const char *aHexString, uint8_t aNumChars)
>+{
>+  for (uint8_t i = 0; i < aNumChars; i++)
[Actually size_t makes more sense for an array index like this.]

>     str.Right(fileNum, 10);
>     if ((fileNum.CharAt(0) == '(') && (fileNum.CharAt(9) == ')'))
>     {
>-      for (i = 1; i < 9; i++)
>+      str.Right(fileNum, 9);
>+      if (MsgIsHex(fileNum.get(), 8))
You can use fileNum.get() + 1 to avoid copying the string again. (The string usage here is terribly archaic but none of the external API developers use a Mac so we don't compile this.)

Comment 30

[:aceman]

(In reply to [e-mail] from comment #29)
> >+  if (PR_sscanf(resultStr.get(), "%x", &result) == 1)
> Isn't 1 the "success" value?
Good catch.

> >+      if (MsgIsHex(token + 1, 2)) {
> >+        // If we got here, we successfully decoded a quoted printable sequence,
> >+        // so bump each pointer past it and move on to the next char.
> >+        dest[destIdx++] = MsgUnhex(token + 1, 2);
> [Given that there are hardly any uses of token left, you might as well just
> use src + srcIdx + 1 directly.]
Yes, except the ugly cast from unsigned char* to const char*.

> >+inline
> >+uint64_t MsgUnhex(const char *aHexString, uint8_t aNumChars)
> [I still think the definition of this belongs in nsMsgUtils.cpp]
> 
> >+  // Large numbers will not fit into uint64_t.
> >+  NS_ASSERTION(aNumChars <= 16, "Hex literal too long to convert!");
> [As you only pass 8 maximum, why bother with uint64_t ?]
In nsMsgDatabase::YarnToUInt64 we pass up to 16.
 
> >     str.Right(fileNum, 10);
> >     if ((fileNum.CharAt(0) == '(') && (fileNum.CharAt(9) == ')'))
> >     {
> >-      for (i = 1; i < 9; i++)
> >+      str.Right(fileNum, 9);
> >+      if (MsgIsHex(fileNum.get(), 8))
> You can use fileNum.get() + 1 to avoid copying the string again. (The string
> usage here is terribly archaic but none of the external API developers use a
> Mac so we don't compile this.)
Neither do I, so I just blindly converted this. Hopefully it compiles on try server.

Comment 31

[:aceman]

Attachment: patch v6

Comment 32

[[e-mail]]

(In reply to aceman from comment #30)
> > [Given that there are hardly any uses of token left, you might as well just
> > use src + srcIdx + 1 directly.]
> Yes, except the ugly cast from unsigned char* to const char*.
Oops, both callers ugly cast from const char* to unsigned char* (which is worse, because it's casting away const!) Obviously that needs to be fixed in a separate bug, but that will therefore simplify these uses too.

> > [As you only pass 8 maximum, why bother with uint64_t ?]
> In nsMsgDatabase::YarnToUInt64 we pass up to 16.
Good catch, I hadn't spotted that. (Too many deleted lines I guess!)

Comment 33

[[e-mail]]

Comment on attachment 8424458 [details] [diff] [review]
patch v6

>+inline
>+uint64_t MsgUnhex(const char *aHexString, uint8_t aNumChars)
[Actually I meant that all of these sizes should be size_t rather than uint8_t]
[I still think the definition of this belongs in nsMsgUtils.cpp]

>+      if (MsgIsHex(fileNum.get() + 1, 8))
[It occurs to me that there is a way of writing this that might be less confusing by changing the Msg*ex functions to take a const nsACString& parameter, although all the callers would have to start using Substring (Substring(fileNum, 1, 8) here and Substring(pointer, length) for the other callers). Msg*hex could then retrieve the pointer using BeginReading.]

Comment 34

[:aceman]

(In reply to [e-mail] from comment #33)
> >+inline
> >+uint64_t MsgUnhex(const char *aHexString, uint8_t aNumChars)
> [Actually I meant that all of these sizes should be size_t rather than
> uint8_t]
> [I still think the definition of this belongs in nsMsgUtils.cpp]
Ok, done.

> >+      if (MsgIsHex(fileNum.get() + 1, 8))
> [It occurs to me that there is a way of writing this that might be less
> confusing by changing the Msg*ex functions to take a const nsACString&
> parameter, although all the callers would have to start using Substring
> (Substring(fileNum, 1, 8) here and Substring(pointer, length) for the other
> callers). Msg*hex could then retrieve the pointer using BeginReading.]
As all callers are char* for now, I don't like to promote them to nsCString or something. That can be done in another bug if really needed.

Comment 35

[:aceman]

Attachment: patch v6.1

Thanks.

Comment 36

[:aceman]

Actually this needs a try run to check the changes in the Mac import that none of us built. Aryx, could you do it? All tests too.

Comment 37

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=b23e82fa064d

Comment 38

[:aceman]

Comment on attachment 8425001 [details] [diff] [review]
patch v6.1

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

So it seems to build on Mac. As nobody has yet reviewed this mailnews/import part I'd like to ask standard8 to do it as the current peer.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/753d0e637a7d