1307080 - -Werror=sign-compare: mailnews/compose/src/nsMsgSendLater.cpp may need a few patches.

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[ISHIKAWA, Chiaki]

Recently, promoting of compiler warning as error has propagated to try-comm-central build and I have found a few extra patches to C-C tree is in order.

I have other patches that may have fixed the issue while I worked on others, but at least this single line patch is additionally necessary to get past by the -Werror=sign-compare
*IF* I apply -Werror=sign-compare all over the trees (actually now the option is
selectively applied to subdirectories).

Anyway, this one liner should not hurt.

Attachment comes in the next post.

TIA

Comment 1

[ISHIKAWA, Chiaki]

Attachment: mailnews-compose-sign-compare.patch

A one liner patch.
(There may be other places but they are buried in other patches, I am afraid.)

TIA

Comment 2

[:aceman]

Comment on attachment 8797120 [details] [diff] [review]
mailnews-compose-sign-compare.patch

Jorg, can you check this?

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8797120 [details] [diff] [review]
mailnews-compose-sign-compare.patch

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

I don't thing this is formally the right thing to do although it might work.

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +986,5 @@
>        break;
>      case 'X': case 'x':
>        {
>          int32_t headLen = PL_strlen(HEADER_X_MOZILLA_STATUS2);
>          if (headLen == end - buf &&

Note: Here we calculate a strlen and assign to an int32_t.

@@ +989,5 @@
>          int32_t headLen = PL_strlen(HEADER_X_MOZILLA_STATUS2);
>          if (headLen == end - buf &&
>            !PL_strncasecmp(HEADER_X_MOZILLA_STATUS2, buf, end - buf))
>            prune_p = true;
> +        else if (strlen(HEADER_X_MOZILLA_STATUS) == (size_t) (end - buf) &&

The difference between two pointers is an integer, see:
http://www.cs.yale.edu/homes/aspnes/pinewiki/C(2f)Pointers.html
The question is only, which type that integer should have.
I don't know why this line isn't simply changed to:
  PL_strlen(HEADER_X_MOZILLA_STATUS) == end - buf &&
following the example above.

(size_t) (end-buf) could be problematic if buf>end. From the snippet presented here I cannot see that end>=buf is guaranteed.

Or how about:
buf + strlen(HEADER_X_MOZILLA_STATUS) == end?
Pointer + number of elements gives new pointer.

Comment 4

[Jorg K (CEST = GMT+2)]

The other option would be:
(int32_t) strlen(HEADER_X_MOZILLA_STATUS) == end - buf
Surely the length of the status string won't overflow an int32_t ;-)

I still think (with a "k", not as I wrote with a "g")
buf + strlen(HEADER_X_MOZILLA_STATUS) == end
is best.

Comment 5

[ISHIKAWA, Chiaki]

Attachment: mailnews-compose-sign-compare.patch

(In reply to Jorg K (GMT+2) from comment #4)
> The other option would be:
> (int32_t) strlen(HEADER_X_MOZILLA_STATUS) == end - buf
> Surely the length of the status string won't overflow an int32_t ;-)
> 
> I still think (with a "k", not as I wrote with a "g")
> buf + strlen(HEADER_X_MOZILLA_STATUS) == end
> is best.

I took up the approach of "buf + strlen(HEADER_X_MOZILLA_STATUS) == end" in this new patch.

BTW, I am wondering.

Isn't mailnews/compose covered by -Werror=sign-compare check?
It is an important part of C-C tree, but come to think of it,
I did not seem to get this check/error on try-comm-central and only
with my local build where I enabled -Werror=sign-compare all over the C-C and M-C tree.

TIA

Comment 6

[Jorg K (CEST = GMT+2)]

Comment on attachment 8798197 [details] [diff] [review]
mailnews-compose-sign-compare.patch

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

Thanks, we'll need to fix the commit message before landing this. I'll take care of it.

Comment 7

[ISHIKAWA, Chiaki]

Attachment: mailnews-compose-sign-compare.patch (take3)

Sorry, I uploaded a bad patch.
This one should compile OK.
TIA

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8798198 [details] [diff] [review]
mailnews-compose-sign-compare.patch (take3)

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

Yes, you fixed the parenthesis.

Comment 9

[:aceman]

(In reply to ISHIKAWA, Chiaki from comment #5)
> Isn't mailnews/compose covered by -Werror=sign-compare check?

Is it in the default warning flags? If not, then we may not have it enabled.

> It is an important part of C-C tree, but come to think of it,
> I did not seem to get this check/error on try-comm-central and only
> with my local build where I enabled -Werror=sign-compare all over the C-C
> and M-C tree.

It may also be you are using gcc 6 but the servers still use 4.8.

Comment 10

[Jorg K (CEST = GMT+2)]

Comment on attachment 8798198 [details] [diff] [review]
mailnews-compose-sign-compare.patch (take3)

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

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +986,5 @@
>        break;
>      case 'X': case 'x':
>        {
>          int32_t headLen = PL_strlen(HEADER_X_MOZILLA_STATUS2);
>          if (headLen == end - buf &&

Why don't we fix this while we're here to make it consistent:
if (buf + strlen(HEADER_X_MOZILLA_STATUS2) == end &&

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: mailnews-compose-sign-compare.patch (v4).

Sorry for hijacking this.

I think the right way to fix this is to just remove the meaningless pointer arithmetic. This was meant as some ill-guided optimisation, like: If the length isn't right, no need to do the expensive string compare. Sadly, if you look at it, there are heaps of string compares in this if-statement.

Comment 12

[:aceman]

Comment on attachment 8798349 [details] [diff] [review]
mailnews-compose-sign-compare.patch (v4).

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

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +985,5 @@
>        header = &m_to;
>        break;
>      case 'X': case 'x':
>        {
> +        if (!PL_strncasecmp(HEADER_X_MOZILLA_STATUS2, buf, end - buf))

Can't this change behaviour? What is the result if buf is the header string (mozilla-status2) plus some suffix? Will the strings be considered equal? Or if buf is a substring of mozilla-status2?

It seems to me the usual usage of PL_strncasecmp in c-c is of this pattern:
PL_strncasecmp(buf, HEADER_X_MOZILLA_STATUS2, HEADER_X_MOZILLA_STATUS2_LEN) .
Thus you have a variable length buffer and comparing it to a constant string for the strings number of characters. If you see https://dxr.mozilla.org/comm-central/rev/e8fa13708c070d1fadf488ed9d951464745b4e17/mozilla/nsprpub/lib/libc/src/strcase.c#67 it seems it only checks string *a if it hit \0 *run out of chars), not string 'b'. I'm not sure there what happens if the max is more than the length of 'b' and 'a' is also longer than 'b'.
Can you debug the place and output what kind of strings we get here?

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: mailnews-compose-sign-compare.patch (v5).

I'm going back to Chiaki-san's solution, but with the lines above fixed as well. OK? I don't feel like debugging this so shave off one comparison.

Comment 14

[:aceman]

Comment on attachment 8798603 [details] [diff] [review]
mailnews-compose-sign-compare.patch (v5).

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

OK.

Comment 15

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/86ced6e7669214ed2e09b49aaa4bb366b864e66c