Mail pegs a core in FindURL

[humdinger]

This is hrev52597, 32bit.

The attached email hangs Mail in the "reader" thread in FindURL() at ​Content.cpp#n324.

I've attached the mail that triggers it.

This is the string being processed by FindURL():

https://github.com/pelya/xserver-xsdl. To me if we have some kind of forum
to talk about this i could raise the topic there; I cannot work alone on
big projects like that. So I will get straight into the point:
 - Does core team have plans to support Xlib

The text in the email is:

https://github.com/pelya/xserver-xsdl. To me if we have some kind of forum
to talk about this i could raise the topic there; I cannot work alone on
big projects like that. So I will get straight into the point:
 - Does core team have plans to support Xlib, pulseaudio etc or where does
these discussions take place?

So it seems to get stuck after removing the "," after the "Xlib"... (?)

[humdinger]

Hanging email file

[nzimmermann]

There's no way not to hang in that code path: (Content.cpp#n324.) 'suffix' is not updated in the loop:

	// filter out some punctuation marks if they are the last character
	char suffix = str[urlLength - 1];
	while (suffix == '.'
			|| suffix == ','
			|| suffix == '?'
			|| suffix == '!'
			|| suffix == ':'
			|| suffix == ';')
		urlLength--;

It should read:

	// filter out some punctuation marks if they are the last character
	char suffix = str[urlLength - 1];
	while (suffix == '.'
			|| suffix == ','
			|| suffix == '?'
			|| suffix == '!'
			|| suffix == ':'
			|| suffix == ';') {
		urlLength--;
		suffix = str[urlLength - 1];
            }

But that's dangerous as urlLength could be <= 0. Better version:

	// filter out some punctuation marks if they are the last character
        assert(urlLength > 0); // Is that guaranteed? Otherwise the next line would crash, so I suppose callers never pass in urlLength=0.
	char suffix = str[urlLength - 1];
	while (suffix == '.'
			|| suffix == ','
			|| suffix == '?'
			|| suffix == '!'
			|| suffix == ':'
			|| suffix == ';') {
		urlLength--;
		if (urlLength == 0)
		    break;
		suffix = str[urlLength - 1];
            }

Can you verify this humdinger?

[humdinger]

That does indeed work, thanks! Wanna submit a patch for that?

[nzimmermann]

Yes, I will do it this week -- quite busy at work at the moment.

[owenca]

I would modify the code thus:

	if (urlString == NULL)
		return type;

	// find the end of the URL based on word boundaries
	const char* str = string.String() + urlPos;
	urlLength = strcspn(str, " \t<>)\"\\,\r\n");

	// filter out some punctuation marks if they are the last character
	while (urlLength > 0) {
		char suffix = str[urlLength - 1];
		if (suffix != '.'
				&& suffix != ','
				&& suffix != '?'
				&& suffix != '!'
				&& suffix != ':'
				&& suffix != ';')
			break;
		urlLength--;
	}

	*urlString = BString(string.String() + urlPos, urlLength);

	return type;
}

[humdinger]

@nzimmermann: If you don't find the time, maybe Owen can propose his solution as a patch at Gerrit? I run into this eternal loop of doom pretty regularly... :)

[waddlesplash]

Merged in hrev52659, with one minor change: we can't quick-exit if urlString == NULL, as the caller probably wants the correct urlLength.

[owenca]

@waddlesplash: good catch! I must've been using Java too much.

[nielx]

Assign tickets with status=closed and resolution=fixed within the R1/beta2 development window to the R1/beta2 Milestone