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

[Stu Tomlinson]

On Tue, 2008-06-24 at 13:20 +0300, Felipe Contreras wrote: 
> 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.

I don't think this is a buffer overflow. I suspect it is actually going
to (attempt) to read the next 2 bytes from the servconn file descriptor,
not a regular buffer (and I don't think reading past the end of a buffer
is a buffer overflow either).

<snip code> 
> 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.

No, it means if an error command has a numeric parameter, it is assumed
it has a payload. If the error has no parameters at all, nothing bad
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)

Indeed it was.

> 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.

Felipe's never seen an error with a payload, so Stu was clearly making
things up, deliberately broke MSNP14 in the process, intentionally added
a confusing commit message explaining that some error messages can have
a payload too, all with the goal of breaking shit and leaving it to rot
for a year.

> 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.

Thanks for pointing out the correct way to fix this, and special thanks
to Elliott for actually doing it.

Regards,


Stu.