170416 - nsIOutputStream::Write() users must null-terminate output.

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Johnny Stenback (:jst)]

Since nsIOutputStream::Write() is a scriptable method all users of this method
*must* make sure that the string that they're writing out is null-terminated,
and also that it does *not* contain embedded null characters. This is kinda lame
since this interface could be used to write out any kind of data, and that's
fine since the Write() method does take a count argument as well, but if the
consumer is JS then there's no way for the consumer to get any data past the
first null, and there's no way to limit the data to the number of written
characters by any other means than by null-terminating the output (failure to do
so is likely to cause a crash).

One of the places where this is violated, and can easily be triggered is in
ConvertAndWrite() in nsDocumentEncoder.cpp, patch to fix that is coming up.

Comment 1

[Darin Fisher]

wow!  so we really screwed up when we froze this interface then because there's
no way to guarantee that the data will not have embedded null characters.  the
string is simply containing raw bytes.  are there any slight modifications that
we can make for the sake of xpconnnect/js that won't break C++ API
compatibility?  e.g., can we say something like length_is or size_is in the
appropriate way?  (i'm reaching here, but...)

Comment 2

[Darin Fisher]

or perhaps we should introduce nsIScriptableOutputStream and deprecate
nsIOutputStream::Write for use by JS code.  is the XML http request interface
which depends on nsIOutputStream frozen?

Comment 3

[Johnny Stenback (:jst)]

Attachment: Fix nsDocumentEncoder.cpp

Comment 4

[Doug Turner (:dougt)]

the interface says nothing about embedding nulls.  We can have two impls. one
for no-non-null chars, another for embedded nulls.  Hell, the impl can do this.

Comment 5

[Darin Fisher]

is nsDocumentEncoder the only implementation that really needs to be fixed?
...i'm guessing that this issue will continually come back to bite us :(

Comment 6

[Bradley Baetz (:bbaetz)]

Can't we use size_is on the string, in the IDL (assuming that that works)? That
won't affect c++, although it would change the typelib, I presume, which
probably is still technically the changing of the frozen interface.

Although you wouldn't notice the difference in any situation where it works
currently, would you?

Comment 7

[Doug Turner (:dougt)]

Index: nsIOutputStream.idl
===================================================================
RCS file: /cvsroot/mozilla/xpcom/io/nsIOutputStream.idl,v
retrieving revision 3.9
diff -u -r3.9 nsIOutputStream.idl
--- nsIOutputStream.idl	3 May 2002 07:49:24 -0000	3.9
+++ nsIOutputStream.idl	26 Sep 2002 00:32:14 -0000
@@ -111,7 +111,7 @@
      *   block the calling thread (non-blocking mode only)
      * @throws <other-error> on failure
      */
-    unsigned long write(in string aBuf, in unsigned long aCount);
+    unsigned long write([size_is(aCount)] in string aBuf, in unsigned long aCount);
 
     /**
      * Writes data into the stream from an input stream.

Comment 8

[Bradley Baetz (:bbaetz)]

Yeah, but the question still is:

a) 'is this changing a frozen interface in a way which anyone whose code
previously worked could notice'?

b) Does this actually work to fix jst's problem - noone else that I/dbradley
could find used this attribute on strings, although there is typelib code for it.

Also, are there any other affected methods (nsIInputStream::Read?)

Comment 9

[Doug Turner (:dougt)]

1) depends if it changes the js api semantecs
2) i posted it here so jst can test it
3) nsIInputStream is marked noscript.  nsIScriptableInputStream has this problem.

Comment 10

[Darin Fisher]

we have a choice.  either we try to fix this _or_ we say that JS code will only
work with text data.  if you think about it, the latter choice just means we'd
need to introduce something like a scriptable output stream that doesn't suck. 
it's not such a bad alternative is it?

Comment 11

[Doug Turner (:dougt)]

jst, rpotts, and I kicked around a few idea.  One of them was a
nsIString(In|Out)Stream with read/write a AString or ACString.  This would allow
handling of nulls.  

Comment 12

[Bradley Baetz (:bbaetz)]

I think we should really prefer fixing the existing interface if possible,
rather than creating a new interface, and having to update code everywhere to
get it to work...

Comment 13

[Doug Turner (:dougt)]

I agree with the first part.  However, there isn't going to be any adjusting if
we move to a new interface.

Comment 14

[Bradley Baetz (:bbaetz)]

dougt: But you'd have to adjust all the users which talk to js - see jst's
original patch, and the original motivation for this

Comment 15

[Johnny Stenback (:jst)]

Mass-reassigning bugs to [e-mail]

Comment 16

[Fabian Guisset]

the patch in this bug (from jst) is still relevant and has never been checked in.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 100329 [details] [diff] [review]
Fix nsDocumentEncoder.cpp

>       char finish_buf[32];
>       charLength = 32;

Probably make this:

char finish_buf[33];
charLength = sizeof(finish_buf) - 1;

and r+sr=dbaron

Comment 18

[Johnny Stenback (:jst)]

Fix checked in.