104876 - c:foo wrongly considered valid URL

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. Read plaintext email with "c:foo" in content

Actual result:
It's linked to <c:foo> (which means, given the recognizer code, that Necko
considers "c:foo" to be a valid URL, which is of course wrong).
Expected result:Only valid and usable URI schemes are considered valid.

Comment 1

[Andreas Otte]

Nice example ... :-) 

I guess this depends on the recognizer code. The urlparser will make this:

urltest -std c:foo
c:foo
c,,,foo,-1,/,,,,,,c://foo/

which is somewhat legal, since the parser does not know which schemes are vaild.

A creation of a real url would give these results:

urltest c:foo
c:foo
Can not create URL                                                                  

So it seems that the code that does this recognition inside mailnews has to do
something more than just call the parser, at least, if not creating the full
url, it has to check for a valid protocolhandler. This can be a performance hit.
Where is the recognizer code located? I will take a look.

Comment 2

[Ben Bucksch (:BenB)]

> A creation of a real url

That's what I do:
 rv = mIOService->NewURI(specStr, nsnull, getter_AddRefs(uri));
<http://lxr.mozilla.org/seamonkey/source/netwerk/streamconv/converters/mozTXTToHTMLConv.cpp#380>
> would give these results: [...] Can not create URL
It doesn't, which is the bug.

Comment 3

[Andreas Otte]

This is really strange. I used the exact same codelayout inside urltest.cpp:

        result = pService->NewURI(i_pURL, nsnull, getter_AddRefs(pURL));

which returns the results as seen above. I will dig into this.

Comment 4

[Olav Vitters]

See also bug 104693 (and the C:\\ example)

Comment 5

[R.K.Aa.]

this is a dup of bug 104693. Punctuation liks dots and colons that don't have
space before and after are treated like http:// URL's

Comment 6

[Andreas Otte]

I will be gone for two days, can't work on it now, but there seems a lot of
coverage on this stuff. This seems to be a recent change, but going through
changes in netwerk over the last week nothing comes to mind except the str?cmp
changes by alecf. Or maybe some caching of protocol handlers gone bad ... 

Comment 7

[Ben Bucksch (:BenB)]

> going through changes in netwerk over the last week nothing comes to mind
> except the str?cmp changes by alecf.

Same with the converter.

Comment 8

[Gervase Markham [:gerv]]

This isn't a dupe of the recent regression; it's been this way since 2001-09-03
at least.

Gerv

Comment 9

[Andreas Otte]

It seems we call in these cases a default protocol handler which calls the os to
figure out if it can handle the url. Somehow, I don't know why yet, "c" is
considered a protocol that can be executed by (at least) linux. As a consequence
the url can be created ...

Comment 10

[Andreas Otte]

No, it's GetCachedProtocolHandler in nsIOService.cpp. It mistakes c for the
chrome protocol and returns the chrome handler. 

Comment 11

[Andreas Otte]

This funny thing was caused by rjesup's checkin to nsIOService.cpp (1.104)

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsIOService.cpp&root=/cvsroot&subdir=mozilla/netwerk/base/src&command=DIFF_FRAMESET&rev1=1.103&rev2=1.104

protocol "c" is found to be "chrome" because of the strncasecmp with length 1. 

Comment 12

[Andreas Otte]

Attachment: patch to fix the problem ... have to check for the length too ...

Comment 13

[Andreas Otte]

Attachment: this is better ...

Comment 14

[Andreas Otte]

And now is clear why urltest worked ... there was no chrome url to cache ...

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 54246 [details] [diff] [review]
this is better ...

Doh!
r=[e-mail]

Comment 16

[Darin Fisher]

Attachment: alternate solution w/o the additional strlen

Comment 17

[Andreas Otte]

Comment on attachment 54502 [details] [diff] [review]
alternate solution w/o the additional strlen

gScheme[i][len] might peek into some memory that is somewhere else if you enter a very long scheme to test with. I can't think of a case where this check fails but some boundary checking code might get upset.

Comment 18

[Darin Fisher]

actually, i believe it is ok.  consider these 3 cases:

1) len > strlen(gScheme[i]) and gScheme[i] is a substring of scheme: strncasecmp
would return non-zero, since gScheme[i][k] would equal '\0' for some k < len.

2) len < strlen(gScheme[i]) and scheme is a substring of gScheme[i]: strncasecmp
would return zero, but gScheme[i][len] would be non-zero.

3) len == strlen(gScheme[i]) and scheme is equal to gScheme[i]: strncasecmp
would return zero, and gScheme[i][len] would be zero.

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 54502 [details] [diff] [review]
alternate solution w/o the additional strlen

r=[e-mail]
I agree with darin's analysis of the latest patch

Comment 20

[Andreas Otte]

I also believe the patch works fine, it's just that for example with a test
scheme of "ccccccccccccccccccccccccccccccccccccc" and a list of cached hadlers
like "http","chrome","file" we would peek with gScheme[i][len] into some memory
that is not allocated by the gScheme array. If you both think that is okay, I
will accept that.

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

cccccccccccccccccccccccccccccccc won't cause a past-end reference.

Looking at it again, chromeccccccccccccccccccccccccc will, however.
While this is 99% likely to be "safe" (not cause a page fault), it's not 100% -
if the linker puts the string near the end of a page it could fault.

Also, even worse, gScheme[i][len] could be a \0 in ANOTHER string (or other
data-section constant).  So this is inherently wrong.  I retract my r= for the
no-strlen patch.

If you want to avoid the strlen you should encode an array of length of the
values  in gScheme (gSchemeLen[]).  Otherwise do the strlen - the data will be
in L1 cache, and the longest scheme is about 6 characters, so the hit is minimal
(possible cache miss for strlen's code if the compiler doesn't inline it).

Comment 22

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 54502 [details] [diff] [review]
alternate solution w/o the additional strlen

My apologies - it's too early in the morning.  I looked at it one more time, and realized that the strncasecmp uses len.  Since we don't look at gSchemes[i][len] unless strncasecmp returns 0, we know that the scheme and gScheme[i] are the same for len characters.  Thus we know that strlen(gScheme[i]) >= len, so we can look at gScheme[i][len].  Mea Culpa.  I need caffiene

Comment 23

[Andreas Otte]

Okay, I didn't took the execution order into account. I will go hunting for sr=,
but it will be weekend before I check this in, I'm away for three days.

Comment 24

[Darin Fisher]

yes, the code working correctly depends on execution order, and rjesup explained
it perfectly.

Comment 25

[Doug Turner (:dougt)]

Comment on attachment 54502 [details] [diff] [review]
alternate solution w/o the additional strlen

okay by me

Comment 26

[Randell Jesup [:jesup] (needinfo me)]

So we have r=/sr=, let's check in

Comment 27

[Andreas Otte]

fix checked in ...

Comment 28

[esther]

Using build 20020322 on winxp, mac 9.1 and linux and the original scenario in
plain text compose and html compose, this is fixed. Verified