330503 - Address misc issues with nsICMSMessage2.idl

Status: RESOLVED FIXED

(MailNews Core :: Security: S/MIME, defect)

Description

[Kai Engert [:KaiE:]]

Addressing of some review comments in bug 111384, that request changes to IDL files, has been postponed.


in nsICMSMessage2.idl

> Instead of UnsignedCharPtr, maybe you want this:
> 
>   void asynchVerifyDetachedSignature(in nsISMimeVerificationListener listener,
>                                      [array, length_is(aDigestDataLen)]
>                                      in octet aDigestData,
>                                      in unsigned long aDigestDataLen);

    * We are using "native unsigned char" ptr, because the function 
    * signatures of this one and nsICMSMessage::verifyDetachedSignature 
    * should be the identical. Cleaning up nsICMSMessages needs to be
    * postponed, because this asynch version is needed on MOZILLA_1_8_BRANCH.
    *
    * Once both interfaces get cleaned up, the function signature should
    * look like:
    *     [array, length_is(aDigestDataLen)]
    *     in octet aDigestData,
    *     in unsigned long aDigestDataLen);

> Is there a reason why these interfaces are declared non-scriptable?
> It is usually best to make as much scriptable as possible.

 * This interface is currently not marked scriptable,
 * because its verification functions are meant to look like those
 * in nsICMSMessage. At the time the ptr type is eliminated in both 
 * interfaces, both should be made scriptable.


in nsIX509Cert2.idl and nsIX509Cert3.idl

> >+interface nsIX509Cert3 : nsISupports {
> >+
> >+};
> 
> When extending an existing interface with the same root name, it is
> usually a good idea to try to extend directly from it.  Doing so
> avoids the overhead of an additional vtable pointer per object.
> So, in this case, I'd probably consider extending from nsIX509Cert2,
> but I notice that nsIX509Cert2 does not extend nsIX509Cert :-(
> Maybe that should be changed.

Because I need this patch on 1.8 branch, I added this
comment to the interface.
 * TODO: nsIX509Cert3 should be derived from nsIX509Cert2
 *       (and nsIX509Cert2 derived from nsIX509Cert)

Note that nsIX509Cert2 can NOT land on 1.8 branch.

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

    *     [array, length_is(aDigestDataLen)]

I'd add a const too ([const, array, length_is(aDigestDataLen]), assuming the semantics of this function allow it.

Comment 2

[Kai Engert [:KaiE:]]

reassign bug owner.
mass-update-kaie-20120918

Comment 3

[:Cykesiopka]

Bug 643041 merged nsIX509Cert2 and nsIX509Cert3 into nsIX509Cert.
Bug 611752 + Bug 1027241 effectively moved nsICMSMessage2.idl and related code to c-c.

... so I will move this bug over so c-c people can decide whether they want to do this.

Comment 4

[Magnus Melin [:mkmelin]]

Noticed this looking at bug 1551704. Ben, maybe we can get rid of this "array" too.

Comment 5

[Ben Campbell]

Attachment: 330503-use-arrays-in-nsICMSMessage-1.patch

Mostly simple, but a few little cleanups, mainly to the error handling. The error handling ones could do with a second opinion. eg:

• there was a place where PR_GetError() was used instead of checking the nsresult to a XPCOM call.
• there was a call to nsIOutputStream::write() where I removed the check on the number of bytes written, on the grounds that a truncated write is an error anyway (nsIOutputstream doesn't seem explicit about this, but the PR_Write that underlies a lot of outputstream implementations is).

try build looks sane:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=37a45a3b79d56509f3a20dac4f5b8ca13512fafa

Comment 6

[Kai Engert [:KaiE:]]

Wow. Thanks for getting to that very old bug that I've filed 14 years ago.

The patch looks mostly good, except the two places that you've pointed out.

(In reply to Ben Campbell from comment #5)

Mostly simple, but a few little cleanups, mainly to the error handling. The error handling ones could do with a second opinion. eg:

• there was a place where PR_GetError() was used instead of checking the nsresult to a XPCOM call.

The related underlying NSS API calls are HASH_Begin, HASH_Update and HASH_End, all of them have return type void, and nsCryptoHash doesn't check for PR_GetError(), so potentially it might be necessary.
We should investigate, I've filed bug 1609449.

• there was a call to nsIOutputStream::write() where I removed the check on the number of bytes written, on the grounds that a truncated write is an error anyway (nsIOutputstream doesn't seem explicit about this, but the PR_Write that underlies a lot of outputstream implementations is).

The API description explicitly allows a partial write:
https://searchfox.org/mozilla-central/rev/9b4b41b95cbcda63f565bdc24411e15248f91d83/xpcom/io/nsIOutputStream.idl#78

If the current implementation cannot handle a partial write, then we should keep the current check for the number of bytes written.

The place you've changed (and several other unchanged places in that file) all return a special value of -1, casting to nsresult. I don't know if that special result value is consumed and expected by any callers. Maybe we should keep this result code, until we've been able to verify that no callers (up the caller chain) expect a -1 on failure.

The try build might not be a good way to answer that question, we don't have much test coverage for S/MIME processing failures.

Comment 7

[Kai Engert [:KaiE:]]

If you keep the existing check for PR_GetError, and keep returning that failure on a short write, I can give you r+ immediately.

If you want to clean up those details, we could have a follow-up bug. If you file one, it should suggest that all uses of MK_MIME_ERROR_WRITING_FILE to be replaced (after code investigation).

Comment 8

[Ben Campbell]

Attachment: 330503-use-arrays-in-nsICMSMessage-2.patch

Thanks Kai - exactly the second opinion I was looking for!

Updated patch, with restored PR_GetError() and partial-write check.
With the PR_GetError() case I did add some code to set a failing nsresult if there was an error (previously I think a PR error would have caused and early exit returning NS_OK).

Forgot to mention it earlier, but I removed the goto FAILs just because they didn't like me adding the a new digest local mid-scope. Nothing particularly ideological, but there was no cleanup code, so seemed cleanest to just bail out immediately.

(I've reset the r?, but the only changes are in mailnews/extensions/smime/src/nsMsgComposeSecure.cpp).

Comment 9

[Ben Campbell]

(In reply to Kai Engert (:KaiE:) from comment #6)

The API description explicitly allows a partial write:
https://searchfox.org/mozilla-central/rev/9b4b41b95cbcda63f565bdc24411e15248f91d83/xpcom/io/nsIOutputStream.idl#78

Huh. My initial reading of that made it seem more ambiguous. But yeah, it seems pretty clear that non-error truncated writes should be expected.

If the current implementation cannot handle a partial write, then we should keep the current check for the number of bytes written.

Should we instead be looping around counting bytes until they've all been sent (or an error occurs)?
In what circumstances should truncated writes be expected? I'm guessing some network-based stream implementations...
(there are about 7 uses of Write() in nsMsgComposeSecure, and the looping code isn't any bigger than the existing truncated-write check. But I'd be willing to bet that there are a whole heap of other such cases to check in c-c... is this worth addressing?)

The place you've changed (and several other unchanged places in that file) all return a special value of -1, casting to nsresult. I don't know if that special result value is consumed and expected by any callers. Maybe we should keep this result code, until we've been able to verify that no callers (up the caller chain) expect a -1 on failure.

Written up in Bug 1609549.

Comment 10

[Kai Engert [:KaiE:]]

(In reply to Ben Campbell from comment #9)

Should we instead be looping around counting bytes until they've all been sent (or an error occurs)?
In what circumstances should truncated writes be expected? I'm guessing some network-based stream implementations...
(there are about 7 uses of Write() in nsMsgComposeSecure, and the looping code isn't any bigger than the existing truncated-write check. But I'd be willing to bet that there are a whole heap of other such cases to check in c-c... is this worth addressing?)

Difficult to say if looping is safe. I'd be reluctant to touch this without a good understanding of the consequences, given our priorities might not allow us to to spend much time on S/MIME.

Comment 11

[Kai Engert [:KaiE:]]

Comment on attachment 9121159 [details] [diff] [review]
330503-use-arrays-in-nsICMSMessage-2.patch

thanks, r=kaie

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/bbea4c75d982
Use Array<octet> for passing byte buffers in nsICMSMessage.idl. r=kaie