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

Stu Tomlinson stu at nosnilmot.com
Wed Jul 9 21:22:49 EDT 2008

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

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



More information about the Devel mailing list