384210 - Investigate nsIMAPBodypartMessage string initiation copies a literal in nsImapServerResponseParser::bodystructure_data()

Status: NEW

(MailNews Core :: Backend, defect)

Description

[Nick Kreeger]

For some reason, the caller of the nsImapServerResponseParser constructor actually copies a literal. 

This needs to be investigated to see why the code was written this way:
    nsIMAPBodypartMessage *message = new nsIMAPBodypartMessage(NULL, NULL, PR_TRUE,
                                                               strdup("message"), strdup("rfc822"),
                                                               NULL, NULL, NULL, 0);

Comment 1

[Mark Banner (:standard8)]

Flagging as a possible memory leak investigation.

Comment 2

[Wayne Mery (:wsmwk)]

related to bug 1285434 or bug 1285098  ?

Comment 3

[ISHIKAWA, Chiaki]

OK, I will take a look at this next week.

Comment 4

[ISHIKAWA, Chiaki]

Here is what I found out.

If I remove strdup() call from the two arguments, I get the following warning from GCC.
(This warning from GCC is probably an error to a strictly conforming compiler.)

/NREF-COMM-CENTRAL/comm-central/mailnews/imap/src/nsImapServerResponseParser.cpp
: In member function ‘virtual void nsImapServerResponseParser::bodystructure_dat
a()’:
/NREF-COMM-CENTRAL/comm-central/mailnews/imap/src/nsImapServerResponseParser.cpp
:2707:71: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwr
ite-strings]
                                 fServerConnection.GetPreferPlainText());
                                                                       ^
/NREF-COMM-CENTRAL/comm-central/mailnews/imap/src/nsImapServerResponseParser.cpp
:2707:71: warning: ISO C++ forbids converting a string constant to ‘char*’ [-Wwr
ite-strings]

So, I am checking a patch that creates a char array that contains the strings and pass them to the function.

TIA

Comment 5

[ISHIKAWA, Chiaki]

But if I pass an array that is NOT allocated, I get an error from
memory free function later.


But if I pass an array that is NOT allocated, I get an error from
memory free function later.

As it turnes out the bodyType and bodySubType paramters are released
when
the nsIMAPBodyPartLeaf object is destroyed and thus the parameters
must be dynamically allocated  character strings.

///////////// nsIMAPBodypartMessage ////////////////////////

nsIMAPBodypartMessage::nsIMAPBodypartMessage(char *partNum,
                                             nsIMAPBodypart *parentPart,
                                             bool topLevelMessage,
                                             char *bodyType, char *bodySubType,
                                             char *bodyID,
                                             char *bodyDescription,
                                             char *bodyEncoding,
                                             int32_t partLength,
                                             bool preferPlainText)
 : nsIMAPBodypartLeaf(partNum, parentPart, bodyType, bodySubType, bodyID,
                      bodyDescription, bodyEncoding, partLength,
                      preferPlainText)

Then there is another superclass:


nsIMAPBodypartLeaf::nsIMAPBodypartLeaf(char *partNum,
                                       nsIMAPBodypart *parentPart,
                                       char *bodyType, char *bodySubType,
                                       char *bodyID, char *bodyDescription,
                                       char *bodyEncoding, int32_t partLength,
                                       bool preferPlainText)
  : nsIMAPBodypart(partNum, parentPart), mPreferPlainText(preferPlainText)


And when nsIMAPBodypart is destroyed:

nsIMAPBodypart::~nsIMAPBodypart()
{
  PR_FREEIF(m_partNumberString);
  PR_FREEIF(m_contentType);
  PR_FREEIF(m_bodyType);
  PR_FREEIF(m_bodySubType);
  PR_FREEIF(m_bodyID);
  PR_FREEIF(m_bodyDescription);
  PR_FREEIF(m_bodyEncoding);
  PR_FREEIF(m_partData);
  PR_FREEIF(m_headerData);
  PR_FREEIF(m_boundaryData);
}



So these members need to be dynamically allocated.
QED.

Comment 6

[ISHIKAWA, Chiaki]

Attachment: Clarify why |strdup| calls are necessary.

I suggest to clarify the comment regarding this issue.

TIA

Comment 7

[Kent James (:rkent)]

Comment on attachment 8887843 [details] [diff] [review]
Clarify why |strdup| calls are necessary.

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

You are explaining why the strdup is needed (and not a memory leak) as written, but not really explaining why the code needs to be written that way. I don't believe that it does.

It seems to me like the correct approach is to make those two parameters const char* instead of char*, then removing both the strdup as well as release in the destruct or. Other places where m_bodyType or m_bodySubType are set would need the same treatment.

But I easily get confused in all of this const nonsense, so correct me if I am wrong.

Comment 8

[ISHIKAWA, Chiaki]

(In reply to Kent James (:rkent) from comment #7)
> Comment on attachment 8887843 [details] [diff] [review]
> Clarify why |strdup| calls are necessary.
> 
> Review of attachment 8887843 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> You are explaining why the strdup is needed (and not a memory leak) as
> written, but not really explaining why the code needs to be written that
> way. I don't believe that it does.
> 

Now I see what the original question was asking.
I only thought the question was asking why strdup() was necessary.
That was simple, but no, I now realize the real question/request is NOT that simple.

> It seems to me like the correct approach is to make those two parameters
> const char* instead of char*, then removing both the strdup as well as
> release in the destruct or. Other places where m_bodyType or m_bodySubType
> are set would need the same treatment.
> 
> But I easily get confused in all of this const nonsense, so correct me if I
> am wrong.

OK, my take on this is: there are simply too many places where PL_strdup() and friends are used to 
cope with the parameters that are freed on the class object destruction. 
The design seems to assume
the allocation on the user side and release on the object destruction.

Since there are simply too many calls of PL_strdup() and friends involved
to change the behavior of destruction to release the passed members and not releasing them, 
I will come back to this when other bugzilla issues are solved (and not exactly sure when.).

Comment 9

[Wayne Mery (:wsmwk)]

(In reply to ISHIKAWA, Chiaki from comment #8)
> ...
> Since there are simply too many calls of PL_strdup() and friends involved
> to change the behavior of destruction to release the passed members and not
> releasing them, 

Are you suggesting an comprehensive overhaul of these calls? In a new bug?

Comment 10

[ISHIKAWA, Chiaki]

(In reply to Wayne Mery (:wsmwk) from comment #9)
> (In reply to ISHIKAWA, Chiaki from comment #8)
> > ...
> > Since there are simply too many calls of PL_strdup() and friends involved
> > to change the behavior of destruction to release the passed members and not
> > releasing them, 
> 
> Are you suggesting an comprehensive overhaul of these calls? In a new bug?

I believe that I mentioned this for this particular file/function.

I will look into this after the M-C/C-C layout change is over (from the previous C-C/M-C layout). Almost finished, but somehow I have uncovered a host of source code issues. One is potentially crash-inducing bug :-(

Comment 11

[Wayne Mery (:wsmwk)]

(In reply to ISHIKAWA, Chiaki from comment #10)

I will look into this after the M-C/C-C layout change is over (from the
previous C-C/M-C layout). Almost finished, but somehow I have uncovered a
host of source code issues. One is potentially crash-inducing bug :-(

Is there a bug number for this?

Comment 12

[ISHIKAWA, Chiaki]

(In reply to Wayne Mery (:wsmwk) from comment #11)

(In reply to ISHIKAWA, Chiaki from comment #10)

I will look into this after the M-C/C-C layout change is over (from the
previous C-C/M-C layout). Almost finished, but somehow I have uncovered a
host of source code issues. One is potentially crash-inducing bug :-(

Is there a bug number for this?

I am afraid I have forgotten which bug it was.
But I believe I have filed them four years ago.