[PATCH 2/4] Use NULL instead of "offline" for the user status.

[Felipe Contreras]

On Tue, Jun 24, 2008 at 11:29 AM, Jorge Villaseñor <salinasv at gmail.com> wrote:
> 2008/6/24 Richard Laager <rlaager at wiktel.com>:
>> On Tue, 2008-06-24 at 01:15 -0400, Elliott Sales de Andrade wrote:
>>> so I haven't seen any show-stoppers lately.
>>
>> Does that mean that if we merged your changes, we could ship MSNP15
>> enabled by default?
>>
>> Richard
>>
>
> I'm testing qulogic's branch HEAD and hope the crash was fixed. I'm
> not really sure, it looks like some memory corruption.
>
> I think it's safe to get out 2.4.3 as a bugfix release and soon 2.5.0
> as a great featurefull release with msnp15 enabled by default and all
> that new cool shiny features.

That's not a proper fix, but should reduce the possibility of the crash.

Now, I just found the real issue:

(From Masca's log)
(13:44:26) msn: C: SB 370: CAL 2 foo at hotmail.com
(13:44:26) msn: S: SB 370: 217 2
(13:44:26) MSNP14: get payload len:2

See the problem? "212 2" Means error on transaction #2, but somehow
the code is assuming there's a payload following. So what happens is
that it will try to read 2 more bytes from the buffer and save it to
the payload. Since there aren't any more bytes there's a buffer
overflow and the heap gets corrupted.

The offending code comes from command.c:

static gboolean
msn_check_payload_cmd(const char *str)
{
	g_return_val_if_fail(str != NULL, FALSE);

	if((!strcmp(str,"ADL")) ||
		(!strcmp(str,"GCF")) ||
		(!strcmp(str,"SG")) ||
		(!strcmp(str,"MSG")) ||
		(!strcmp(str,"RML")) ||
		(!strcmp(str,"UBX")) ||
		(!strcmp(str,"UBN")) ||
		(!strcmp(str,"UUM")) ||
		(!strcmp(str,"UBM")) ||
		(!strcmp(str,"FQY")) ||
		(!strcmp(str,"UUN")) ||
		(!strcmp(str,"UUX")) ||
		(!strcmp(str,"IPG")) ||
		(is_num(str))){
			return TRUE;
		}

	return FALSE;
}

That means that thanks to is_num(str), each and every error command
sent by the server is considered to have a payload, and if it doesn't;
bad stuff happens.

It was introduced in 71ad73bcb02d2d2f7263d02abf77df511c4e8f8c by
nosnilmot on May 27 2007:
Fix & tidy up payload length detection for commands (errors can have
payloads too)

That's complete and utterly wrong, and I don't know how nobody else
noticed after more than one year. Any time an error comes without
payload, bang! And by the way, I don't think I've ever seen an error
with a payload.

By the way, the whole msn_check_payload_cmd introduced by mayuan goes
against my design; the payload_len field should be set by the command
handler, only after processing the command, so that all cases are
handled.

This generalization is completely wrong.

But what do I know, you are the ones in charge and I guess arguing
against my code-style is more important than getting me to fix the
code the way I want. Who would want that!

-- 
Felipe Contreras