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

[Felipe Contreras]

On Thu, Jul 10, 2008 at 4:22 AM, Stu Tomlinson <stu at nosnilmot.com> wrote:
> 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).

I'm not sure what it is, but it definitely causes a heap corruption.

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

Most error commands have a numeric parameter. [1]

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

It's great to have maintainers who know what they are doing, and take
the issues seriously.

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

Unfortunately if indeed there are error commands with payload there
still will be issues.

[1] http://www.hypothetic.org/docs/msn/reference/error_list.php

-- 
Felipe Contreras