1921950 - C-C TB passes nullptr to memcpy() https://searchfox.org/comm-central/source/mailnews/mime/src/mimetext.cpp#395

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[ISHIKAWA, Chiaki]

This is a follow up to Bug 1852662 .

In the proposed patch which is in C-C tree, there is a problem.
We are passing a nullptr to the second argument of memcpy.
https://searchfox.org/comm-central/source/mailnews/mime/src/mimetext.cpp#395

memcpy(rotated_line, line, length);

I think the second argument |line| should read |original_line|.

There are two problems.

• We do not have a test in mochitest/xpcshell-test that exercises this code.
  We would have caught the issue if we had.
• The code is incorrect.

Then how have I found the problem? Simple. GCC 14 found the problem.
I compile C-C TB for local testing using gcc, especially when I need to check C-C TB under valgrind although this combination has ceased to execute for the last few months for no apparent reason.
I wanted to compile C-C TB using gcc after using clang to create SANITIZED versoin for testing for a couple of months,
since Wayne asked me to see if the problem in Bug 1611708 still persists. This can only be tested by valgrind.

But when I tried compiling using gcc for the first time in the last several weeks, I get the following error.

                 from /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/mime/src/mimetext.cpp:15:
In function ‘void* memcpy(void*, const void*, size_t)’,
    inlined from ‘int MimeInlineText_rotate_convert_and_parse_line(const char*, int32_t, MimeObject*)’ at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/mime/src/mimetext.cpp:398:11:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:33: error: argument 2 null where non-null expected [-Werror=nonnull]
   29 |   return __builtin___memcpy_chk (__dest, __src, __len,
      |          ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~
   30 |                                  __glibc_objsize0 (__dest));
      |                                  ~~~~~~~~~~~~~~~~~~~~~~~~~~
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:29:33: note: in a call to built-in function ‘void* __builtin_memcpy(void*, const void*, long unsigned int)’
cc1plus: all warnings being treated as errors
gmake[4]: *** [/NEW-SSD/NREF-COMM-CENTRAL/mozilla/config/rules.mk:676: mimetext.o] Error 1

I checked the code, and now I believe |line| ought to read |original_line|.
I have no idea why clang did not complain.

Another thing is where is this "rot13" usage inside TB documented?
This is important if someone wants to create a test to exercise this piece of code.
I could not find one in easy-to-access places and thus, this was the reason the original patch in Bug 1852662 I created did not try to
handle the in-place replacement of the data, which would have made the patch much cleaner as BenC pointed out, and this is indeed what Kaie did.

Do we need rot13 functionality in there?
All I could find about rot13 and thunderbird is that someone posted a feeler for an extension that would use rot13 and other simple ciphers. But if the addon does that, we don't need that function within C-C TB, do we?

EDIT: formating issue.

Comment 1

[Magnus Melin [:mkmelin]]

I believe, https://en.wikipedia.org/wiki/ROT13 though I don't think that's used for much in practice

Comment 2

[Kai Engert [:KaiE:]]

Thank you for finding this mistake!

Comment 3

[Kai Engert [:KaiE:]]

I cannot find any code to handle a ROT13 Content-Transfer-Encoding.

The processing here is controlled by variable ro13_p.
I cannot find any code that sets rot13_p to true.

I went back to a local copy of the old Mozilla CVS, AFAICT it never sets rot13_p to true either.

Let's remove the rot13 code.

Comment 4

[Kai Engert [:KaiE:]]

Attachment: Bug 1921950 - Remove unused code for ROT13 encoding. r=mkmelin

Comment 5

[ISHIKAWA, Chiaki]

Thank you for removing this strange code!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9bc81b08b3c3
Remove unused code for ROT13 encoding. r=mkmelin