Refactor telnet socket read for unit testing / fuzzing #2337
Conversation
|
Hey there! Thanks for helping Mudlet improve. 🌟 You can directly test the changes here:
No need to install anything - just unzip and run. |
src/ctelnet.cpp
Outdated
| readSocket(in_buffer, amount); | ||
| } | ||
|
|
||
| void cTelnet::readSocket(char*& in_buffer, int amount) { |
There was a problem hiding this comment.
Cosmetic - but can we have the { on the next line please? 👼
Less cosmetic but why have in_buffer at all now? As in_bufferx is a char array using in_bufferx as readSocket(in_bufferx, amount) is the more normal way of passing C string pointers around - though the signature of readSocket should be amended to (void) cTelnet::readSocket(char*, size_t)...! Of course since socket.read(...) can return a qint64 value of -1 on errors we really should not be calling the new method at all if amount is negative right there...
There was a problem hiding this comment.
Feedback applied!
Amount is checked for -1 in the readSocket() a couple of lines below, so it's OK. Might as well have the check there because fuzzing could try that amount and we should be ready for it.
| } | ||
|
|
||
| void cTelnet::readSocket(char*& in_buffer, int amount) { | ||
| mpHost->mInsertedMissingLF = false; |
There was a problem hiding this comment.
If it is revised to bail out on error then this will need to go back in before that point.
There was a problem hiding this comment.
Does it? The old code did not.
There was a problem hiding this comment.
I meant, if you went with - what I think is a better arrangement - to not call readSocket(...) if amount is negative (i.e. in an error condition) then this line would need to be executed back in handle_socket_signal_readyRead(...) first.
There was a problem hiding this comment.
Why is it a better arrangement? The function already checks if the amount is negative. Why check twice?
There was a problem hiding this comment.
Well if we change the confusing name (readSocket is not reading the socket - that has already been done before it is called - perhaps it should be processSocketData 😉 ) then does not executing processSocketData should amount have the sentinel -1 (or indeed any negative value) seem like a more sane approach.
Given that amount refers to the size of a char array then using the size_t (which, being intended for indexing into arrays is the correct) type to quantify it in the call to what I am calling processSocketData. By checking amount to be a non-negative you have bounds checked that it can be static_cast<size_t>(...) from the qint64 type that socket.read(...) returns into the argument that processSocketData wants; there is no need to then check it and return prematurely from that method...
There was a problem hiding this comment.
Fuzzing works by generating random inputs - so we want to check the size of our inputs within processSocketData, and and not another funciton, because fuzzing will be calling processSocketData and not another function.
There was a problem hiding this comment.
I am not sure I am getting the full picture of how fuzzing will be applied here but this revision does not worsen (except by adding the overhead of another function call) the code so I guess it is okay to apply it given that you seem to have an idea why it is needed.
Brief overview of PR changes/additions
Adjust the socket read function to take its data in as parameters.
Motivation for adding to Mudlet
So we can enable fuzzing or unit testing on it.
Other info (issues closed, discussion etc)
Hopefully this doesn't break anything, this is very important code. Doesn't seem to with a quick test.