Issue 621 in webp: BuildHuffmanTable could replace assert with if() for safety

[jongr… via monorail]

Status: New
Owner: ----
Labels: Type-Defect Priority-Medium

New issue 621 by jongr...@gmail.com: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621

This is a suggestion to replace the assert() with if, so it's safer in such cyber and safety critical code systems

1. git clone https://chromium.googlesource.com/webm/libwebp/ webp_test
webp_test/src/utils/hjuffman_utils.c

2. Better to check these at runtime, and return 0 if invalid. This function is called rarely, there won't be a perceptible impact compared to the benefit of the code being safer.

assert(code_lengths_size != 0);
assert(code_lengths != NULL);
assert((root_table != NULL && sorted != NULL) ||
(root_table == NULL && sorted == NULL));
assert(root_bits > 0);

Change to this:
if(code_lengths_size == 0) return 0;
if(code_lengths == NULL) return 0;
if((root_table != NULL && sorted != NULL) ||
(root_table == NULL && sorted == NULL)) return 0;
if(root_bits < 0) return 0;


3.


What version of the product are you using? On what operating system?
Latest git source

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[jz… via monorail]

Updates:
Owner: vra...@google.com

Comment #1 on issue 621 by jz...@google.com: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621#c1

This is an internal function. The assertions are meant to ensure that the callers invoke it correctly. We tend to use assertions for impossible conditions in this code base, supported by analysis and test coverage. If any are possible then of course they should be made into proper checks.

[jongr… via monorail]

Comment #2 on issue 621 by jongr...@gmail.com: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621#c2

I would agree, yes asserts for impossible conditions are good. Until the point when they are not impossible and occur, in which case it crashes. The cost of putting in a an if() is only a branch, this function is called rarely, it would be good to add these if() after the asserts, so no user is punished. "Failed to load image" is much nicer than a user suffering a crash. Safety critical code would add the checks, the compiler can optimize them out if they are truly not needed.

I'm going to open another ticket, found some places where the compiler can't be sure the bounds are not overrun, so an if() check is needed.

[jz… via monorail]

Comment #3 on issue 621 by jz...@google.com: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621#c3

Often there are known assumptions or checks done at higher levels that allow lower level functions to omit them and to simply assert to prevent code changes from breaking the assumption. If you can show that the assumptions are incorrect and want to post a patch for discussion, please have a look at our contributors guide [1].

[1] https://chromium.googlesource.com/webm/libwebp/+/05c469843ce3947a63f5ac7ac2c3baa5df2c167d/CONTRIBUTING.md

[jongr… via monorail]

Comment #4 on issue 621 by jongr...@gmail.com: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621#c4

Ok, I will prepare a patch. I'm building latest code. SDL2 is the current release, I get a build error for the old version of SDL. Is the project updating to SDL2 soon?

$ make -f makefile.unix all
<snip>
extras/vwebp_sdl.c:33:10: fatal error: SDL/SDL.h: No such file or directory
33 | #include <SDL/SDL.h>
| ^~~~~~~~~~~

[pasca… via monorail]

Comment #5 on issue 621 by pasca...@gmail.com: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621#c5

Hi,

let's be clear: i don't think creating the patch is the hardest task here.

The real endeavor is making sure that these 'if's' will catch something the 'assert()' wouldn't.

[j… via monorail]

Comment #6 on issue 621 by j...@jguk.org: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621#c6

Pascal, you're completely right, the patch is the easy part.

My notes below, due to points (5) and (6) particularly - I would recommend putting in the check. I've included my build output where two of the asserts appear to be needed to be if() in this investigation I did.


When I consider Formal Verification for libraries like this, I would think if the code meets the requirements of Functional Safety for a Safety Critical System. Where a Safety Critical System could be decoding WebP files for an automotive display, aviation, defence, health care.

Considering the code, there are a few areas I would focus on :-

1. Testing, so while there is assert() present in Release builds (!NDEBUG), it depends what WebP files are tested in such a build (we couldn't link to a build in safety critical software that used assert() because it might call abort()).
2. If WebP files in the wild tested (perhaps as part of integration testing)
3. Files that haven't been created yet, someone could create a file in 2024 that overflows a buffer in a 2023 released version of WebP decoder.
4. The 'cost' of a branch instruction (not jumped), typically one cycle. It's a very low cost, for 100% certainty that the software will detect an invalid parameter. It feels like premature optimization to take out such checks.
5. We don't want to wait for corrupt files to crash safety critical systems, or cyber security exploits, it's safer just to be 100% careful in the code we can control.
6. Does the compiler optimize out the checks? It doesn't, which means it's not dead code. There would be no 'cost' if the optimizer removed the checks.

Due to these reasons I would recommend putting in the checks.


src/utils/huffman_utils.c: In function ‘BuildHuffmanTable’:
src/utils/huffman_utils.c:88:84: error: call to ‘_stop_compile’ declared with attribute error: build stopped
88 | #define compile_assert(condition, description) {if(!_check_condition(condition)) { _stop_compile();}}
| ^~~~~~~~~~~~~~~
src/utils/huffman_utils.c:119:3: note: in expansion of macro ‘compile_assert’
119 | compile_assert((root_table != NULL && sorted != NULL) || (root_table == NULL && sorted == NULL), "");
| ^~~~~~~~~~~~~~
src/utils/huffman_utils.c:88:84: error: call to ‘_stop_compile’ declared with attribute error: build stopped
88 | #define compile_assert(condition, description) {if(!_check_condition(condition)) { _stop_compile();}}
| ^~~~~~~~~~~~~~~
src/utils/huffman_utils.c:120:3: note: in expansion of macro ‘compile_assert’
120 | compile_assert(root_bits > 0, "");

[j… via monorail]

Comment #7 on issue 621 by j...@jguk.org: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621#c7

I took a look at the compiler's optimizer output, it can't figure out that 2nd param of BuildHuffmanTable() 'root_bits' is always the macro LENGTHS_TABLE_BITS passed through from VP8LBuildHuffmanTable() in vp8l_dec.c

I suggest just to remove the parameter, and use LENGTHS_TABLE_BITS from huffman_utils.h directly in the function in huffman_utils.c
Would a patch be supported?

[j… via monorail]

Comment #8 on issue 621 by j...@jguk.org: BuildHuffmanTable could replace assert with if() for safety
https://bugs.chromium.org/p/webp/issues/detail?id=621#c8

VP8LBuildHuffmanTable() calls BuildHuffmanTable() with either a 'sorted' buffer on the stack, or a heap allocated 'sorted'

The compiler can't see the sorted[] array accesses are always within bounds, the change I propose is to add a new parameter to pass the sorted_count :

static int BuildHuffmanTable(HuffmanCode* const root_table, int root_bits,
const int code_lengths[], int code_lengths_size,
uint16_t sorted[], const size_t sorted_count)

Then the offset[symbol_code_length] index into sorted[] via the array subscript should be checked before it is accessed.