Hi,

we've found an issue with sieve plugin. When script includes two missing optional scripts, it causes crash.

For reproducing the issue, script names must belong to same hash table node, so either two scripts with same name or scripts where 'hash(name) % table->size' results in same value, like missing_a and missing_aa in reproducer below.

Reduced reproducer:

cat >dovecot.min.conf <

cat >reproducer.sieve <

echo -n > empty

sieve-test -c dovecot.min.conf reproducer.sieve empty

any test mail would do, but empty file is enough for reproducer

# return sblock->id causes the crash as sblock is NULL backtrace: (gdb) bt #0 sieve_binary_block_get_id (sblock=0x0) at src/lib-sieve/sieve-binary.c:421 #1 ext_include_execute_include (renv=0x5555555bbed8, include_id=1, flags=EXT_INCLUDE_FLAG_OPTIONAL) at plugins/include/ext-include-common.c:696 #2 opc_include_execute (renv=0x5555555bbed8, address=0x5555555bbf10) at plugins/include/cmd-include.c:399 #3 sieve_interpreter_operation_execute (interp=0x5555555bbe88) at src/lib-sieve/sieve-interpreter.c:901 #4 sieve_interpreter_continue (interp=0x5555555bbe88, interrupted=0x0) at src/lib-sieve/sieve-interpreter.c:959 #5 sieve_interpreter_start (interp=0x5555555bbe88, result=0x5555555b9230, interrupted=0x0) at src/lib-sieve/sieve-interpreter.c:1049 #6 sieve_interpreter_run (interp=0x5555555bbe88, result=0x5555555b9230) at src/lib-sieve/sieve-interpreter.c:1057 #7 sieve_run (sbin=0x55555558ee18, result=0x5555555b9230, eenv=0x7fffffffe890, ehandler=0x55555558c338) at src/lib-sieve/sieve.c:357 #8 sieve_test (sbin=0x55555558ee18, msgdata=0x7fffffffe9f0, senv=0x7fffffffea30, ehandler=0x55555558c338, stream=0x5555555b2cd0, flags=SIEVE_EXECUTE_FLAG_LOG_RESULT) at src/lib-sieve/sieve.c:598 #9 main (argc=5, argv=0x555555567a10) at src/sieve-tools/sieve-test.c:391

issue seems to be caused by lib-sieve/plugins/include/ext-include-common.c: ext_include_generate_include

on first pass (script) the check for script already compiled into binary at

529: included = ext_include_binary_script_get_include_info(binctx, script);

returns NULL, so code goes through 'else' block where it hits sieve_script_is_open(script) check. It includes script with NULL block, returns 0 so caller cmd_include_generate does not emit anything. On second pass the above 'included' check returns first missing script, it goes through true block where it just checks flags and returns non-zero, so caller this time goes through the emit code block and the previously included NULL sblock gets referenced causing crash later.

Note: despite scripts have different name, they pass the "same script" check because sieve_file_script_equals compares 0==0 dev & inode numbers as scripts do not exist. This is why scripts do not have to have same names. Side effect of this is that as missing_a and missing_aa are treated like the same script, only first one is linked in compiled svbin. Which means that if missing_aa reappears later, svbin won't get recompiled.

Another manifestation of the issue is when included missing_a script is present when main script is compiled and later missina_a is removed again. During execution, script is not recompiled and it aborts for hash table dupe check: sieve_binary_open->ext_include_binary_script_include->hash_table_insert

There are several ways how to solve this. I've tried a few, but there were other corner cases that appeared. One tried way was for example ext_include_generate_include in the 'if (included)' replicate the check sieve_script_is_open(...) from else block, just check flags, no include and set result to 0. Another way to solve this is to return from ext_include_execute_include early if sblock is NULL just before it tries to derefernce it, similarly as it does if it fails 'once' check. Same for dumping in opc_include_dump. Whilst the first approach seems a bit better, it won't stop crashing in existing environments as it won't trigger svbin recompilation and svbin would fail. This means that the second soultion is sort of needed anyway. This still won't solve the problem with different missing scripts as only first one would be able to trigger recompilation when file becomes available. Just including both in the mentioned function won't work either as they would still be evaluated as same and hash table anti-dupe check would trigger abort. So some changes in script comparison seem needed anyway. This is what I've used in my approach. Return early from ext_include_execute_include and opc_include_dump if sblock is NULL and change sieve_file_script_equals to compare also script->location for missing scripts (when inode is 0).

Let me know if you need more information.

Cheers, Michal Hlavinka