Ticket #9483 (closed patch: fixed)
OOM/abort when receiving ICQWebMessage
| Reported by: | darkrain42 | Owned by: | MarkDoliner |
|---|---|---|---|
| Milestone: | 2.5.8 | Component: | ICQ |
| Version: | 2.5.7 | Keywords: | |
| Cc: | yumkam@… |
Description
Quoting from http://pidgin.im/pipermail/devel/2009-May/008227.html (with some formatting changes by me. Yuriy is CCed on this ticket.):
Hello! Few month ago ;-) I've got number of OOM/(segv|abort), and found that when pidgin receive chan4/0x1a/ICQWebMessage, it misparses that as ICQSMS, and dies on out-of memory/sigsegv.
- fixes in byte_stream_getstr: early check len for validity (this will cause error later anyway), and only then allocate memory.
- fixes in incomingim_chan4/case 0x1a: better checks for expected format and errors (and not choke on some unknown gibberish). 3-4. [optional] remove introduced in (01) double checks and cleanup
PS patches checked and works on pidgin-2.5.{1..6}. PPS someone should also think about this:
byte_stream_init(&b, "\7abcde", 6);
len = byte_stream_get8(&b); // ok - len=7
foo = byte_stream_get_str(&b, len); // fails!
num = byte_stream_get32(&b); // reads junk! num = 0x64636261
bar = byte_stream_get_str(&b, num); // DIE! (before my patch) or fail;
// or, in more ``lucky'' case ("\7\1\0\0\0B") may read some unexpected
// gibberish into bar
There are too many places where byte_stream_get* functions used without any check for errors :-\ (from quick glance - no as drastic things[*] as with 4/0x1a, but I don't know code well enough to be sure). Maybe, it make sense to advance buffer to end (b->offset = b->len;) immediately on failed attempt to read something?
[*] that is, with 01_*.patch; without - it would be OOM/die.
