Fix exception when decoded value is too large by doubling buffer #4

karamanolev · 2017-12-21T17:27:20Z

No need for the test files. There's a much simpler reproduction, which I included in the tests.
Removed the test for NoDelta found. This will be covered in the logic for doubling the buffer. This changes the API a bit, since now you are free to get a larger delta than the second argument. However, I think that this is, in fact, the correct behavior from an API standpoint. Imagine you write your code, test it with reasonable inputs, everything works. One day your production code breaks, because of some edge case. I think cases of larger delta than the input should be handled by the client code, if it's actually an issue. Otherwise you will just surprise them badly.
The above actually removes the need for the NoDelta exception, so it's removed as well.
sizeof(int) - 4 is 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.
The sprintf had a buffer overflow vulnerability. Fixed by using snprintf. I agree this can get a string truncated, but is much better than it was before.

codecov · 2017-12-21T17:38:40Z

Codecov Report

Merging #4 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master     #4   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           2      2           
  Lines          42     42           
=====================================
  Hits           42     42

samuelcolvin

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.

samuelcolvin · 2017-12-26T17:22:38Z

xdelta3/_xdelta3.c

    output_buf = main_malloc(output_alloc);
    result = xd3_encode_memory(input_bytes, input_size, source_bytes, source_size,
        output_buf, &output_size, output_alloc, flags);
  } 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;


can you remove comments that no longer make sense.

samuelcolvin · 2017-12-26T17:25:16Z

xdelta3/_xdelta3.c

@@ -21,13 +21,13 @@ static PyObject * xdelta3_execute(PyObject *self, PyObject *args)

  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;


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.

samuelcolvin · 2017-12-26T17:25:38Z

tests/test_main.py

@@ -57,6 +51,14 @@ def test_version():
    xdelta3.print_version()


+def test_huge_output():


Makes much more sense 😄

samuelcolvin · 2017-12-26T17:26:52Z

xdelta3/_xdelta3.c

+  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) {


can you at least add a comment explaining what this is doing please.

samuelcolvin · 2017-12-26T17:27:50Z

xdelta3/_xdelta3.c

    }
  } 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]),


makes sense.

samuelcolvin · 2018-01-19T19:28:47Z

@karamanolev do you want to complete this?

karamanolev · 2018-01-19T22:18:25Z

@samuelcolvin I'll address these soon, hopefully I'll find some time. Sorry.

samuelcolvin · 2018-01-19T22:32:01Z

No problems, thank you.

karamanolev added 2 commits December 21, 2017 19:23

Fix exception when decoded value is too large by doubling buffer

1b733e8

Fix error message in tests

43c2f27

samuelcolvin suggested changes Dec 26, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix exception when decoded value is too large by doubling buffer #4

Fix exception when decoded value is too large by doubling buffer #4

karamanolev commented Dec 21, 2017

codecov bot commented Dec 21, 2017 •

edited

Loading

samuelcolvin left a comment

samuelcolvin Dec 26, 2017

samuelcolvin Dec 26, 2017

samuelcolvin Dec 26, 2017

samuelcolvin Dec 26, 2017

samuelcolvin Dec 26, 2017

samuelcolvin commented Jan 19, 2018

karamanolev commented Jan 19, 2018

samuelcolvin commented Jan 19, 2018

		@@ -57,6 +51,14 @@ def test_version():
		xdelta3.print_version()


		def test_huge_output():

Fix exception when decoded value is too large by doubling buffer #4

Are you sure you want to change the base?

Fix exception when decoded value is too large by doubling buffer #4

Conversation

karamanolev commented Dec 21, 2017

codecov bot commented Dec 21, 2017 • edited Loading

Codecov Report

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin Dec 26, 2017

Choose a reason for hiding this comment

samuelcolvin Dec 26, 2017

Choose a reason for hiding this comment

samuelcolvin Dec 26, 2017

Choose a reason for hiding this comment

samuelcolvin Dec 26, 2017

Choose a reason for hiding this comment

samuelcolvin Dec 26, 2017

Choose a reason for hiding this comment

samuelcolvin commented Jan 19, 2018

karamanolev commented Jan 19, 2018

samuelcolvin commented Jan 19, 2018

codecov bot commented Dec 21, 2017 •

edited

Loading