124987 - ASSERTION: nsDependentString must wrap only null-terminated strings

Status: NEW

(Core :: DOM: Serializers, defect)

Description

[timeless]

relatively clean w32 cvs build from this evening.
I think i was reading npm.beos

nsDependentString::Rebind(const unsigned short * 0x041a1f54, const unsigned 
short * 0x041a1f72) line 86 + 34 bytes
nsDependentString::Rebind(const unsigned short * 0x041a1f54, unsigned int 15) 
line 96
nsDependentString::nsDependentString(const unsigned short * 0x041a1f54, 
unsigned int 15) line 99 + 51 bytes
mozTXTToHTMLConv::ItMatchesDelimited(const unsigned short * 0x041a1f54, int 15, 
const unsigned short * 0x021efa08, int 1, mozTXTToHTMLConv::LIMTYPE 

        !Substring(nsDependentString(aInString, aInLength),
                   (before == LT_IGNORE ? 0 : 1),
                   aRepLen).Equals(nsDependentString(rep, aRepLen),
                                   nsCaseInsensitiveStringComparator())

041A1F34  4C 00 44 00 5F 00 4C 00  L.D._.L.
041A1F3C  49 00 42 00 52 00 41 00  I.B.R.A.
041A1F44  52 00 59 00 5F 00 50 00  R.Y._.P.
041A1F4C  41 00 54 00 48 00 3D 00  A.T.H.=.
041A1F54  2E 00 2F 00 6D 00 6F 00  ../.m.o. aInString==041A1F54  
041A1F5C  7A 00 69 00 6C 00 6C 00  z.i.l.l.
041A1F64  61 00 2D 00 64 00 69 00  a.-.d.i.
041A1F6C  73 00 74 00 3A 00 2E 00  s.t.:... aInString+(aInLength==15)
041A1F74  2F 00 6D 00 6F 00 7A 00  /.m.o.z.
041A1F7C  69 00 6C 00 6C 00 61 00  i.l.l.a.
041A1F84  2D 00 64 00 69 00 73 00  -.d.i.s.
041A1F8C  74 00 2F 00 70 00 6C 00  t./.p.l.
041A1F94  75 00 67 00 69 00 6E 00  u.g.i.n.
041A1F9C  73 00 3A 00 2F 00 62 00  s.:./.b.
041A1FA4  6F 00 6F 00 74 00 2F 00  o.o.t./.
041A1FAC  68 00 6F 00 6D 00 65 00  h.o.m.e.
041A1FB4  2F 00 64 00 65 00 76 00  /.d.e.v.
041A1FBC  65 00 6C 00 6F 00 70 00  e.l.o.p.
041A1FC4  2F 00 51 00 74 00 2F 00  /.Q.t./.
041A1FCC  71 00 74 00 2D 00 32 00  q.t.-.2.
041A1FD4  2E 00 33 00 2E 00 30 00  ..3...0.
041A1FDC  2F 00 6C 00 69 00 62 00  /.l.i.b.
041A1FE4  3A 00 0D 00 0A 00 00 00  :.......

The code should use substring.

LT_DELIMITER, mozTXTToHTMLConv::LIMTYPE LT_ALPHA) line 549 + 315 bytes
mozTXTToHTMLConv::StructPhraseHit(const unsigned short * 0x041a1f54, int 15, 
int 0, const unsigned short * 0x021efa08, int 1, const char * 0x021efa04, const 
char * 0x021ef9ec, nsString & {...}, unsigned int & 0) line 600 + 36 bytes
mozTXTToHTMLConv::ScanTXT(const unsigned short * 0x041a1f34, int 31, unsigned 
int 12, nsString & {...}) line 1052 + 98 bytes
mozTXTToHTMLConv::CalculateURLBoundaries(const unsigned short * 0x041a1f30, int 
93, const unsigned int 33, const unsigned int 14, mozTXTToHTMLConv::modetype 
abbreviated, const unsigned int 2, const unsigned int 90, nsString & {...}, 
nsString & {...}, int & 12, int & 57) line 356
mozTXTToHTMLConv::FindURL(const unsigned short * 0x041a1f30, int 93, const 
unsigned int 33, const unsigned int 14, nsString & {...}, int & 224, int & 
805354861) line 476
mozTXTToHTMLConv::ScanTXT(const unsigned short * 0x041a1f30, int 93, unsigned 
int 14, nsString & {...}) line 1097 + 48 bytes
mozTXTToHTMLConv::ScanTXT(mozTXTToHTMLConv * const 0x04236a70, const unsigned 
short * 0x041a1f30, unsigned int 14, unsigned short * * 0x041a0b10) line 1259
MimeInlineTextPlain_parse_line(char * 0x041a0b90, int 93, MimeObject * 
0x041a0050) line 443 + 62 bytes
MimeInlineText_convert_and_parse_line(char * 0x041a0b90, int 93, MimeObject * 
0x041a0050) line 420 + 20 bytes
MimeInlineText_rotate_convert_and_parse_line(char * 0x05e17028, int 93, 
MimeObject * 0x041a0050) line 550 + 17 bytes
convert_and_send_buffer(char * 0x05e17028, int 93, int 1, int (char *, unsigned 
int, void *)* 0x05a6ef70 MimeInlineText_rotate_convert_and_parse_line(char *, 
int, MimeObject *), void * 0x041a0050) line 168 + 15 bytes
mime_LineBuffer(const char * 0x00ffd5a0, int 93, char * * 0x041a0078, int * 
0x041a0080, unsigned int * 0x041a0088, int 1, int (char *, unsigned int, void 
*)* 0x05a6ef70 MimeInlineText_rotate_convert_and_parse_line(char *, int, 
MimeObject *), void * 0x041a0050) line 255 + 29 bytes
MimeInlineText_parse_decoded_buffer(char * 0x00ffd5a0, int 93, MimeObject * 
0x041a0050) line 336 + 45 bytes
MimeLeaf_parse_buffer(char * 0x00ffd5a0, int 93, MimeObject * 0x041a0050) line 
168 + 20 bytes
MimeMessage_parse_line(char * 0x00ffd5a0, int 93, MimeObject * 0x04236780) line 
226 + 20 bytes
convert_and_send_buffer(char * 0x00ffd5a0, int 93, int 1, int (char *, unsigned 
int, void *)* 0x05a771e0 MimeMessage_parse_line(char *, int, MimeObject *), 
void * 0x04236780) line 168 + 15 bytes
mime_LineBuffer(const char * 0x05e9ebb5, int 1611, char * * 0x042367a8, int * 
0x042367b0, unsigned int * 0x042367b8, int 1, int (char *, unsigned int, void 
*)* 0x05a771e0 MimeMessage_parse_line(char *, int, MimeObject *), void * 
0x04236780) line 255 + 29 bytes
MimeObject_parse_buffer(char * 0x05e9ea70, int 1936, MimeObject * 0x04236780) 
line 255 + 49 bytes
mime_display_stream_write(_nsMIMESession * 0x04162dd0, const char * 0x05e9ea70, 
int 1936) line 860 + 20 bytes
nsStreamConverter::OnDataAvailable(nsStreamConverter * const 0x04231800, 
nsIRequest * 0x08a2a3a0, nsISupports * 0x04230500, nsIInputStream * 0x041908b0, 
unsigned int 0, unsigned int 1936) line 902 + 24 bytes
nsDocumentOpenInfo::OnDataAvailable(nsDocumentOpenInfo * const 0x04230300, 
nsIRequest * 0x08a2a3a0, nsISupports * 0x04230500, nsIInputStream * 0x041908b0, 
unsigned int 0, unsigned int 1936) line 241 + 46 bytes
nsStreamListenerTee::OnDataAvailable(nsStreamListenerTee * const 0x04237e50, 
nsIRequest * 0x08a2a3a0, nsISupports * 0x04230500, nsIInputStream * 0x04182880, 
unsigned int 0, unsigned int 1936) line 56 + 51 bytes
nsNNTPProtocol::DisplayArticle(nsIInputStream * 0x08a2dc50, unsigned int 1937) 
line 2554
nsNNTPProtocol::ReadArticle(nsIInputStream * 0x08a2dc50, unsigned int 1937) 
line 2628 + 16 bytes
nsNNTPProtocol::ProcessProtocolState(nsIURI * 0x0678df44, nsIInputStream * 
0x08a2dc50, unsigned int 27997, unsigned int 1937) line 5184 + 19 bytes
nsMsgProtocol::OnDataAvailable(nsMsgProtocol * const 0x08a2a39c, nsIRequest * 
0x08a2da40, nsISupports * 0x0678df40, nsIInputStream * 0x08a2dc50, unsigned int 
27997, unsigned int 1937) line 264 + 32 bytes
nsOnDataAvailableEvent::HandleEvent() line 193 + 70 bytes
nsARequestObserverEvent::HandlePLEvent(PLEvent * 0x0419ddd4) line 116

Comment 1

[Ben Bucksch (:BenB)]

- Component "DOM->Text"
HTML->TTXT != TXT->HTML. Please find the one who made the last string
mass-changes in that class and assign it to him.
- Subject "ASSERTION: nsDependentString must wrap only null-terminated strings"
I have no idea how a 0 could come into a string there. The string data want
through c-strings before, so it cannot contain 0s. So, there must be something
going horribly wrong.

Comment 2

[timeless]

and the winner is ...

Comment 3

[Boris Zbarsky [:bzbarsky]]

...aaand back to default owner.  I changed Compare() to Equals() without
touching any of the strings the comparison was being performed on.  If
construction of those strings is failing...

Steps to reproduce would be good.

Also, Ben, the assertion does _not_ mean there are 0's in the data.  It means
that the last character in the string is _not_ 0.  In other words, you're
passing it a length that's not long enough or something along those lines. For
example, |nsDependentString("aaa", 2)| would give you that assertion.

Comment 4

[Ben Bucksch (:BenB)]

I might have written something like that intentionally when I originally wrote
the class. But since then, the string classes (and the string usage) underwent
heavy changes, so it might be that the code was correct back then.

Does the string class handle the case gracefully and just outputs an assertion
for the case this truncation was not intended, or will there be a problem in
release builds?

Comment 5

[timeless]

Am I free to reflow the code as I see fit, or should I make minimal changes? I 
have modified my tree but I'm not certain people would want my changes.

Comment 6

[Ben Bucksch (:BenB)]

Minimal changes, please. Somebody else is modifing that class, too.

I didn't mean to tell you that you should fix it, just find the right assignee :).

Comment 7

[Daniel Bratell]

Didn't someone switch from Subsume strings to nsDependantStrings? That could be 
tha cause. The error is about wrapping a string that doesn't end with a null in 
a nsDependantString. Maybe use Substring instead? Or is the indices wrong so 
that it should be null terminated?

Comment 8

[timeless]

Attachment: simple fix, don't specify lengths

Comment 9

[Ben Bucksch (:BenB)]

I think we can trust the length variable more than we can the 0 in the string.
I.e. your patch probably introduces a bug.
<bz> says "Substring() is your friend"

Comment 10

[jag (Peter Annema)]

Simpler (and better) fix: replace nsDependentString(ptr, len) with
Substring(ptr, ptr + len). Of course, this means you can replace the
Substring(nsDependentString( combo with just one Substring :-)

Comment 11

[timeless]

Attachment: it builds, so perhaps it's what jag wants

Comment 12

[timeless]

is this better?

Comment 13

[Alec Flett]

Comment on attachment 70701 [details] [diff] [review]
it builds, so perhaps it's what jag wants

Ugly.. is inString in the same function as the substring below? because if so
you could simplify the 2nd part a bit
something like
!Substring(inString, (before == LT_IGNORE ? 0 : 1), aRepLen).Equals(..)

and so forth.
The lease you could do is cache the value of (before == LT_IGNORE ? 0 : 1) and
do some better wrapping to make it a bit more readable.

Comment 14

[Ben Bucksch (:BenB)]

> !Substring(inString, (before == LT_IGNORE ? 0 : 1), aRepLen)

Yes, I'd prefer that, too. I wondered, why timeless didn't do this.

Comment 15

[Donnie Cranford]

Applied and compiled clean on Solaris, tried testcase and looked good, 
r=dcran

Comment 16

[timeless]

alecf
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp&rev=1.55&mark=158,545-548#496
the two blocks are in different functions.

beyond that, if someone wants to finish this patch off by picking a formatting,
feel free,

the best i can propose is to hijack
519                     PRUnichar textAfterPos = aInString[aRepLen + (before ==
LT_IGNORE ? 0 : 1)];

If that were a PRUnichar* then things might naturally be cleaner.

Comment 17

[Ben Bucksch (:BenB)]

> > !Substring(inString, (before == LT_IGNORE ? 0 : 1), aRepLen)
> 
> Yes, I'd prefer that, too. I wondered, why timeless didn't do this.

Because there is not such Substring() :-(.

r=BenB for the latest patch. If you want, you can create a variable for that if
expression, e.g.
  PRUnichar text0 = aInString[0];
  PRInt32 leadchars = (before == LT_IGNORE ? 0 : 1); // bad var name
  PRUnichar textAfterPos = aInString[aRepLen + leadchars];
[...]
        !Substring(aInString + leadchars,
                   aInString + leadchars + aRepLen).Equals(
                            Substring(rep, rep + aRepLen),
                            nsCaseInsensitiveStringComparator())
but my r is valid regardless.

I thought about it and assuming that the last paragraph of comment 3 is correct,
the assertion does *not* hint at a problem. aInString/aInLength is not the full
line (as passed to ScanTXT), but already a substring of it.

Comment 18

[jag (Peter Annema)]

PRUnichar* start = aInString + (before == LT_IGNORE ? 0 : 1);
Substring(start, start + aReplen)

Comment 19

[timeless]

i'm glad benb finally understood the length stuff.

jag: substring is in the middle of one extremely ugly if (...||(...&&...)||...) 
block, do you really want me to try to impose useless instructions for this 
presumably edge case (yes i've made a lot of assertions but so far people 
have accepted them with time)?

I think it /might/ be possible to use a comma operator to setup the start 
prunichar.

fwiw you missed the fun fireworks (and i missed my train).

so far the only change i would be willing to make is leadchars since there are 
imo enough consumers to warrant that.  I think I'd do that given approval from 
an sr, w/o writing a new patch showing my usage, if an someone (e.g. an sr) 
would like to specify spelling, naming, or casing for that variable, feel free.

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.