On Fri, 29 Jun 2012, Samuel Pitoiset wrote:
On Fri, Jun 29, 2012 at 2:06 PM, Martin Storsjö <[email protected]> wrote:
On Thu, 28 Jun 2012, Samuel Pitoiset wrote:
+void ff_rtmpe_update_keystream(URLContext *h)
+{
+ RTMPEContext *rt = h->priv_data;
+ char buf[RTMP_HANDSHAKE_PACKET_SIZE];
+
+ av_rc4_crypt(&rt->key_in, buf, buf, sizeof(buf), NULL, 1);
+ av_rc4_crypt(&rt->key_out, buf, buf, sizeof(buf), NULL, 1);
+ rt->handshaked = 1;
+}
Umm, what does this really do? buf is uninitialized here. If you want to get
random data, get a proper random seed and use some random number generator
(like the lfg stuff) to fill it with actual random data.
It's needed for updating the RC4 keys.
That's not a valid answer. I asked "what does this really do", "It's
needed" is not a valid answer to that question.
You need to learn how to motivate and explain the code you submit. If you
just copied it from somewhere, you'll have to read it until you understand
what it does. Submitting code you don't understand is seldom a good idea.
I said it seems to me that buf is uninitialized here. As far as I can see,
you en/decrypt a buffer with uninitialized data. Initially, this sounded
very very wrong to me.
After re-reading RC4 I now remember that the encryption state isn't
affected by the previous data encrypted. So what this does is that it
consumes 1536 bytes of the RC4 random bytestream. This actually reads the
uninitialized bytes in buf, the xor them with the RC4 bytestream, and
write it back. The only thing we want to achieve is to skip past 1536
bytes of the RC4 bytestream, we're obviously not interested in their
values, nor are we interested in the encrypted data in buf.
So therefore this code as such is ok, but you will have to be able to
answer this when I ask, not just say "It's needed".
This can be improved a bit by not passing buf as source to the
av_rc4_crypt function, but passing NULL instead. Then you won't read the
uninitialized data from the buffer at all, but just fill the buffer with
the generated RC4 bytestream.
So do that change and add the comment "Skip past 1536 bytes of the RC4
bytestream", then it will be better.
@@ -644,9 +646,9 @@ static int rtmp_handshake_imprint_with_digest(uint8_t
*buf)
int i, digest_pos = 0;
int ret;
- for (i = 8; i < 12; i++)
+ for (i = 772; i < 776; i++)
digest_pos += buf[i];
- digest_pos = (digest_pos % 728) + 12;
+ digest_pos = (digest_pos % 728) + 776;
Why these changes? Aren't these codepaths already used by the normal rtmp
code? Or is it just plain wrong and not validated in practice there? Anyway,
this feels like it should go into a separate commit if that's the case.
These changes are needed for RTMPE, I saw that in librtmp... Anyway,
it works fine with the normal rtmp.
I'll separate it in a new patch.
"The changes are needed for RTMPE" is not a valid answer either, I'm sure
you need it because otherwise it wouldn't be in the patch. But what I want
to know is:
1) What does this do? This is needed for a few reasons: One is so that the
reviewer easily can understand it without having to read up on all details
on all the existing functions that the code calls. Another reason is that
you should explain what you intend the change to do. If someone later
looks at the commit for a reason, e.g. if the commit broke something or
seems buggy, one can see that the actual code isn't exactly what you
explain, one can easily find out that the bug was that the code didn't
match what you meant to do. If you don't explain it, one will never know
whether the code was intentionally done that way, or if it was a bug.
2) Why do you do this? ("Needed for RTMPE" is not a valid answer to that.
Something like "This data is ignored in normal RTMP but actually verified
in RTMPE, and due to that I realized that the existing code is wrong" or
so.)
3) Since this is an existing codepath that you change, you also need to
explain what this change does to the existing use case, does it break
something existing, or motivate this apparently random change doesn't
break the existing use cases (plain RTMP).
In principle every single patch (especially those that change existing
code) need to answer all of these questions in the commit message. If this
is missing, and a reviewer asks about it, you need to be able to produce
answers to these questions, otherwise one cannot easily accept the patch,
because one do not know what effect it has on existing code, thus I
wouldn't be sure it doesn't break anything.
// Martin
_______________________________________________
libav-devel mailing list
[email protected]
https://lists.libav.org/mailman/listinfo/libav-devel
