Fix exception when decoded value is too large by doubling buffer#4
Fix exception when decoded value is too large by doubling buffer#4karamanolev wants to merge 2 commits intosamuelcolvin:masterfrom
Conversation
samuelcolvin
left a comment
samuelcolvin
left a comment
There was a problem hiding this comment.
sorry for slow response.
Overall looks good, mostly trivial things to change except the behaviour when the delta is bigger than the input where I disagree with you.
| } else { | ||
| // output shouldn't be bigger than the original plus the delta, but give a little leeway | ||
| output_alloc = input_size + source_size * 11 / 10; | ||
| output_alloc = (input_size + source_size * 11 / 10) << buffer_double_cnt; |
There was a problem hiding this comment.
can you remove comments that no longer make sense.
| if (action == 0) { | ||
| // if the output would be longer than the input itself, there's no point using delta encoding | ||
| output_alloc = input_size; | ||
| output_alloc = input_size << buffer_double_cnt; |
There was a problem hiding this comment.
I'm afraid I don't agree with this. Sorry.
If I'm getting a delta that is bigger than the input I want to get an error which I have to deal with as soon as possible. Silently "failing" to produce a useful delta is not good.
Please remove this.
| xdelta3.print_version() | ||
|
|
||
|
|
||
| def test_huge_output(): |
| if (result == ENOSPC) { | ||
| // Leave some leeway before we overflow int. This is a bad way, the better way would be to | ||
| // check above when we left shift. | ||
| if (buffer_double_cnt < sizeof(int) - 4) { |
There was a problem hiding this comment.
can you at least add a comment explaining what this is doing please.
| } else { | ||
| char exc_str[80]; | ||
| sprintf(exc_str, "Error occur executing xdelta3: %s", xd3_strerror(result)); | ||
| snprintf(exc_str, sizeof(exc_str) / sizeof(exc_str[0]), |
|
@karamanolev do you want to complete this? |
|
@samuelcolvin I'll address these soon, hopefully I'll find some time. Sorry. |
|
No problems, thank you. |
2 participants
sizeof(int) - 4is horrible, but I'm too lazy ATM to implement the correct solution, which is to check that you didn't overflow the int when left shifting in those 2 places. The error handling logic will be more complicated as well.