Re: patch #4 (Was: [Rhythmbox-devel] music sharing patch #3)



On Tue, 2005-08-23 at 16:46 -0400, Charles Schmidt wrote:

> All set as far as this goes, I think.

I think we still need to check that the codesize matches what we expect
for each type.  For example, say you have this as the sole contents of
the buffer:

<content code:INT64><codesize:1><1 byte>

Note this input contains an invalid codesize; it should be 8.  At least,
that's what I'm assuming the DAAP protocol says, maybe the codesize is
only relevant for _TYPE_STRING, but the present code does rely on it.

So given this input to rb_daap_structure_parse_container_buffer, the
variables initially will be:
l = 0
buf_length = 9

This will pass the if (codesize > buf_length - l - 4 || codesize < 0)
check.  Then going down, we drop to the case RB_DAAP_TYPE_INT64,
and we have l = 8.  But rb_daap_buffer_read_int64 is going to read 8
bytes, and we only have 1 left, so we'll overread the buffer.

My original suggestion for one of your earlier patches was just to check
the buffer size remaining before we do any reading.  I think it will be
equivalent to validate the codesize; this scenario relies on the input
containing an invalid codesize.  After reading through  So in your
current code, you'd change it to look like this:

  case RB_DAAP_TYPE_INT64: {
     gint64 val;
     if (codesize != 8)
       val = 0;
     else
       val = rb_daap_buffer_read_int64(&(buf[l]));
    ...
  }

Does that make sense?

And similarly for the _TYPE_VERSION, _TYPE_INT, etc. cases.

Oh...one other thing I just noticed.  Many parts of the Rhythmbox code
assume that strings are UTF-8.  We should add a call to g_utf8_validate
for the RB_DAAP_TYPE_STRING case.

Attachment: signature.asc
Description: This is a digitally signed message part



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]