Fix TAINTED_SCALAR Coverity issues for pkg_editor.c

[IlanTruanovsky]

Several values that were being read through files were not being checked to see if they meet certain requirements.

For one of the issues, the info.name_length variable was not being checked to see if it was less than the size of name when passed into read_data. This was a simple fix.

For the other TAINTED_SCALAR issue, the info.file_length value was never checked. Since this was being passed into a malloc call, it would be possible for the malloc call to use too much memory if the info.file_length number was purposefully set to be large. There were two possible solutions to this that I could think of

1. Add an upper bound to the size of info.file_length. This maintains much of the same code, but also increases the restrictions of the data being passed into the acl_pkg_unpack_buffer_or_file function.
2. Do not malloc an array of length info.file_length and instead reuse a constant sized array. This is the solution I implemented.

Instead of mallocing and allocating memory, I added a while loop to reuse a constant sized array for multiple iterations. The only problem with this that I can foresee is it taking too much time and having multiple iterations of the while loop. However, when I ran the unit tests with and without this change, it took the same amount of time (although I'm not sure if the unit tests cover this case thoroughly). Are there any issues with this that come to mind, @pcolberg?

The error output is large, so I'll trim it down to just the issue without the code trace.

Fixes:

lib/pkg_editor/src/pkg_editor.c:1632:5:
  Type: Untrusted value as argument (TAINTED_SCALAR)

lib/pkg_editor/src/pkg_editor.c:1681:11:
  Type: Untrusted allocation size (TAINTED_SCALAR)

[pcolberg]

Thanks @IlanTruanovsky. Could you split these into two commits? They fix different issues and one of them is more involved, so you want to be able to review, test, and/or revert the latter individually if it turns out to cause a (performance) regression.

Instead of mallocing and allocating memory, I added a while loop to reuse a constant sized array for multiple iterations. The only problem with this that I can foresee is it taking too much time and having multiple iterations of the while loop. However, when I ran the unit tests with and without this change, it took the same amount of time (although I'm not sure if the unit tests cover this case thoroughly). Are there any issues with this that come to mind, @pcolberg?

Your solution is the right approach; you generally want to avoid allocating an unbounded buffer to copy between two files. The question is which buffer size to pick. read_data() contains a loop as well. Running the unit tests, how often is that inner loop executed in a single call to read_data() using your chunk size of 64 kB? How does it change if you decrease the chunk size?

[pcolberg]

This will need an integration test since pkg_editor is a central component; but please wait with testing until we finish the second review round.

[IlanTruanovsky]

Running the unit tests, how often is that inner loop executed in a single call to read_data() using your chunk size of 64 kB? How does it change if you decrease the chunk size?

I did some testing, seems like the loop in read_data() only ever does 1 iteration regardless of the size of the buffer (even at 1 kB).

[pcolberg]

Thanks @IlanTruanovsky! This is ready for integration testing. (Not sure why GitHub actions did not run. You could try force-pushing a rebase onto main.)

[pcolberg]

@IlanTruanovsky, it looks like the pkg_editor unit test does not provide sufficient coverage to detect the regression introduced (at least on Windows) by this change. Besides debugging the regression, could you see what is missing from the unit test to catch this regression?

[pcolberg]

@IlanTruanovsky From a quick test, it looks like the unit test unpack_buffer is broken and not actually testing acl_pkg_unpack_buffer() correctly:

dd if=/dev/urandom of=lib/pkg_editor/test/test.100M bs=1M count=100
dd if=/dev/urandom of=lib/pkg_editor/test/test.1G bs=1M count=1k

--- a/lib/pkg_editor/test/pkg_editor_test.cpp
+++ b/lib/pkg_editor/test/pkg_editor_test.cpp
@@ -445,6 +445,14 @@ TEST(package, unpack_buffer) {
                                PACK_UNPACK_DIR "/src/pkg_editor.c"));
   CHECK_EQUAL(true, files_same("test/pkg_editor_test.cpp",
                                PACK_UNPACK_DIR "/test/pkg_editor_test.cpp"));
+  CHECK_EQUAL(true, files_same("test/test.1G",
+                               PACK_UNPACK_DIR "/test/test.1G"));
+  CHECK_EQUAL(true, files_same("test/test.100M",
+                               PACK_UNPACK_DIR "/test/test.100M"));
+  CHECK_EQUAL(false, files_same("test/test.100M",
+                               PACK_UNPACK_DIR "/test/test.1G"));
+  CHECK_EQUAL(false, files_same("test/test.1G",
+                               PACK_UNPACK_DIR "/test/test.100M"));
 }
 
 TEST(package, unpack_buffer_stdin) {

ninja && ctest -V -R pkg_editor

5: TEST(package, unpack_buffer)
5: ../../lib/pkg_editor/test/pkg_editor_test.cpp:453: error: Failure in TEST(package, unpack_buffer)
5: 	expected <false>
5: 	but was  <true>
5: 
5:  - 58 ms

The above should pass, but instead fails (and suspiciously quickly for a 1 GB file); it appears files_same() is not working. You can also read up on how error handling for C++ streams work; in particular, exceptions need to be enabled explicitly in the constructor of the stream.

[IlanTruanovsky]

I tried replicating your experiment, but mine passes.

I think you're inserting the 1GB and 100MB files in the wrong place, it should be located under <your build dir>/lib/pkg_editor/test/test/test.100M (notice the double /test directories)

[pcolberg]

I think you're inserting the 1GB and 100MB files in the wrong place, it should be located under <your build dir>/lib/pkg_editor/test/test/test.100M (notice the double /test directories)

Thanks, now it's taking the expected time; either way, files_same is flawed since it should not pass when files are absent. But yes, it does not help with the debugging. I noticed the test files are <64kB and wondered whether larger files would pass.

[pcolberg]

bc7b9f6 is causing a regression when decompressing a zero-length file:

cd build/debug
touch lib/pkg_editor/test/test/test.0
ctest -V -R pkg_editor

5: ../../lib/pkg_editor/src/pkg_editor.c:1684: fopen(.pack_unpack_dir/test/test.0)
5: ../../lib/pkg_editor/src/pkg_editor.c:1705: left_to_read==0 num_to_read==0
5: ../../lib/pkg_editor/src/pkg_editor.c:1537: next_in==0x7ffffffe929f avail_in==10
5: ../../lib/pkg_editor/src/pkg_editor.c:1539: next_out==0x7ffffffecc30 avail_out==0
5: ../../lib/pkg_editor/src/pkg_editor.c:1559: next_in==0x7ffffffe929f avail_in==10
5: ../../lib/pkg_editor/src/pkg_editor.c:1561: next_out==0x7ffffffecc30 avail_out==0
5: ../../lib/pkg_editor/src/pkg_editor.c:1564: inflate(&z_info->strm, Z_NO_FLUSH)==-5
5: ../../lib/pkg_editor/src/pkg_editor.c:1566: next_in==0x7ffffffe929f avail_in==10
5: ../../lib/pkg_editor/src/pkg_editor.c:1568: next_out==0x7ffffffecc30 avail_out==0
5: ../../lib/pkg_editor/src/pkg_editor.c:1575: ret != Z_OK
5: acl_pkg_unpack: Error reading file data for .pack_unpack_dir/test/test.0 from buffer

inflate() returns Z_BUF_ERROR which signals that no progress was possible.

[pcolberg]

So far we have found the following gaps in the pkg_editor test suite:

• files_same does not throw an exception when test files are absent.
• Zero-length uncompressed files are not tested.

[IlanTruanovsky]

@pcolberg thanks for all your debugging! I'll take a look at what needs to be done and commit the necessary changes.

[IlanTruanovsky]

So far we have found the following gaps in the pkg_editor test suite:

• files_same does not throw an exception when test files are absent.
• Zero-length uncompressed files are not tested.

Funnily enough, this check fails when I add a check to see if the file exists. I need to investigate this a bit more.

[pcolberg]

So far we have found the following gaps in the pkg_editor test suite:

• files_same does not throw an exception when test files are absent.
• Zero-length uncompressed files are not tested.

Funnily enough, this check fails when I add a check to see if the file exists. I need to investigate this a bit more.

You could enable exceptions on the input file streams:

static bool files_same(const char *f1, const char *f2) {
  std::ifstream file1(f1, std::ifstream::ate | std::ifstream::binary);
  std::ifstream file2(f2, std::ifstream::ate | std::ifstream::binary);
  file1.exceptions(std::ifstream::failbit | std::ifstream::badbit);
  file2.exceptions(std::ifstream::failbit | std::ifstream::badbit);

std::ifstream::exceptions() immediately throws if the stream is in in an error state, e.g., if the file does not exist. It will also cause subsequent operations, e.g., read to throw an exception on failure. In the unit tests we don't care about handling the exceptions; aborting is fine.

[IlanTruanovsky]

So far we have found the following gaps in the pkg_editor test suite:

• files_same does not throw an exception when test files are absent.
• Zero-length uncompressed files are not tested.

Funnily enough, this check fails when I add a check to see if the file exists. I need to investigate this a bit more.

It seems like CMakeLists.txt was never packed to begin with, so it doesn't exist. It also doesn't exist in the cwd while the unit test is running. I'll go ahead and replace the file name with something that exists.

[pcolberg]

It seems like CMakeLists.txt was never packed to begin with, so it doesn't exist. It also doesn't exist in the cwd while the unit test is running. I'll go ahead and replace the file name with something that exists.

Ideally you could generate the input files with a desired range of sizes on the fly. Internally in the runtime, we can make full use of C++17. This includes std::filesystem::temp_directory_path to create a temporary directory in which you could create the temporary input files with hard-coded names. (Please never mind, that just returns the system's temporary directory.) You could use the filesystem library to create a directory under <BUILD>/test (after clearing the directory using remove_all) and populate the desired files with random content.

[IlanTruanovsky]

@pcolberg I added better unit testing for the unpacking part of the pkg_editor unit tests, including:

• random file generation and checking
• large file generation and checking (10 MB)
• empty file generation and checking

Your review is much appreciated!

[IlanTruanovsky]

@pcolberg It's been a while, but I was able to get this PR to pass, though there was one complication that I'd like your input on.

I'm using this flow to remove the tmp dir when creating the random files during our testing. This code does pass CI, but it did not work when I tested locally on a Windows machine (it errors out with Exit code 0xc0000409). The same happened when I replaced this code with fs::remove_all(tmpdir), which also failed Windows CI (not 100% sure this is the correct workflow run). In both cases, Linux worked fine.

So what should be done about this? We can either:

• Leave it as is now as it passes CI. It could be the case that it did not pass locally due to my Windows machine just being slow and causing errors. Even if my local Windows machine was faster, I don't think we'd want something that only intermittently passes.
• Remove this part of the code completely and document the function's new behavior. The tests still pass anyways, and we regenerate the files every time we call the function.
• Investigate other options for removing the folder. I'm not sure what other options there are, but maybe you have ideas?

I think option 2 is the best, but I'd like to know what you think.

[zibaiwan]

I had an offline discussion with Ilan. Ideally we don't want to create tmp files in the tmp directory during the unit testing without cleaning them up. @tylerzhao7684 , can you please investigate the error code that Ilan posted in the earlier comment to see what that means on the windows machine and how we can avoid it?

We definitely don't want to have intermittent build failures.

You can reach Ilan or me for help! Thanks!

[tylerzhao7684]

Windows unit tests now passing

[zibaiwan]

Can you please do an integration testing as well? Thanks!

[tylerzhao7684]

Can you please do an integration testing as well? Thanks!

It passed

[zibaiwan]

Thanks Tyler. The change looks good to me. Just need a few hw tests to pass and I will merge.

Requested change implemented and Peter's role changed.

[zibaiwan]

Thanks Tyler!