Opened 10 months ago

Closed 9 months ago

Last modified 9 months ago

#14746 closed bug (fixed)

Mail pegs a core in FindURL

Reported by: humdinger Owned by: czeidler
Priority: normal Milestone: Unscheduled
Component: Applications/Mail Version: R1/Development
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: All

Description

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"... (?)

Attachments (1)

[haiku] some questions from a complete newbe 20181207160619 Abdurrahim Cakar (13.0 KB ) - added by humdinger 10 months ago.
Hanging email file

Download all attachments as: .zip

Change History (8)

comment:1 by nzimmermann, 10 months ago

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?

comment:2 by humdinger, 10 months ago

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

comment:3 by nzimmermann, 9 months ago

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

comment:4 by owenca, 9 months ago

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;
}

comment:5 by humdinger, 9 months ago

@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... :)

comment:6 by waddlesplash, 9 months ago

Resolution: fixed
Status: newclosed

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

comment:7 by owenca, 9 months ago

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

Note: See TracTickets for help on using tickets.