RFE: add transaction support to the libseccomp API

[pcmoore]

This PR contains two patches, the first fixes an existing problem with transaction management and the second adds a new transaction API to libseccomp.

@drakenclimber when you have the chance please take a look and let me know what you think, especially regarding the API as that is difficult/impossible to change later. If everything looks okay, I'll submit another PR with the "src/db.c" fix for the release-2.5 branch,

[drakenclimber]

Thanks for the patchset, @pcmoore. I think the user-facing API is good. I do have a few questions here and there but nothing earth shattering.

[drakenclimber]

Valgrind output of the test I added:

==1041378== Memcheck, a memory error detector
==1041378== Copyright (C) 2002-2022, and GNU GPL'd, by Julian Seward et al.
==1041378== Using Valgrind-3.21.0 and LibVEX; rerun with -h for copyright info
==1041378== Command: ./62-sim-arch_transactions -b
==1041378==
--1041378-- WARNING: unhandled amd64-linux syscall: 317
--1041378-- You may be able to write your own handler.
--1041378-- Read the file README_MISSING_SYSCALL_OR_IOCTL.
--1041378-- Nevertheless we consider this a bug.  Please report
--1041378-- it at http://valgrind.org/support/bug_reports.html.
==1041378== Invalid read of size 8
==1041378==    at 0x406A09: db_col_transaction_commit (db.c:2616)
==1041378==    by 0x402930: seccomp_transaction_commit (api.c:831)
==1041378==    by 0x401375: main (62-sim-arch_transactions.c:92)
==1041378==  Address 0x4a4e9c0 is 0 bytes after a block of size 16 alloc'd
==1041378==    at 0x484782C: calloc (vg_replace_malloc.c:1554)
==1041378==    by 0x4064EB: db_col_transaction_start (db.c:2451)
==1041378==    by 0x4028D9: seccomp_transaction_start (api.c:806)
==1041378==    by 0x401345: main (62-sim-arch_transactions.c:85)
==1041378==
==1041378==
==1041378== HEAP SUMMARY:
==1041378==     in use at exit: 312 bytes in 3 blocks
==1041378==   total heap usage: 102 allocs, 99 frees, 9,864 bytes allocated
==1041378==
==1041378== 312 (32 direct, 280 indirect) bytes in 1 blocks are definitely lost in loss record 3 of 3
==1041378==    at 0x484782C: calloc (vg_replace_malloc.c:1554)
==1041378==    by 0x406543: _db_init (db.c:873)
==1041378==    by 0x406543: db_col_transaction_start (db.c:2465)
==1041378==    by 0x4028D9: seccomp_transaction_start (api.c:806)
==1041378==    by 0x401345: main (62-sim-arch_transactions.c:85)
==1041378==
==1041378== LEAK SUMMARY:
==1041378==    definitely lost: 32 bytes in 1 blocks
==1041378==    indirectly lost: 280 bytes in 2 blocks
==1041378==      possibly lost: 0 bytes in 0 blocks
==1041378==    still reachable: 0 bytes in 0 blocks
==1041378==         suppressed: 0 bytes in 0 blocks
==1041378==
==1041378== For lists of detected and suppressed errors, rerun with: -s
==1041378== ERROR SUMMARY: 2 errors from 2 contexts (suppressed: 0 from 0)

[drakenclimber]

Well unsurprisingly these are the lines that valgrind is angry about:

		/* NOTE: while we release the filters we no longer need, we
		 *       don't bother to resize the filter array, we just
		 *       adjust the filter counter, this *should* be harmless
		 *       at the cost of a not reaping all the memory possible */

		do {
			_db_release(snap->filters[snap->filter_cnt--]);
		} while (snap->filter_cnt > col->filter_cnt);

[drakenclimber]

As expected, resizing the snap filter array appeases valgrind. I feel like it's a worthwhile change since this is likely an infrequent operation (and perhaps only in an error path). I think proper memory management may outweigh speed in this case.

Also, I don't claim to like my implementation - it was just a proof of concept to verify the root cause :).

@@ -2612,8 +2613,17 @@ void db_col_transaction_commit(struct db_filter_col *col, bool user)
                 *       adjust the filter counter, this *should* be harmless
                 *       at the cost of a not reaping all the memory possible */
                do {
+                       struct db_filter **dbs;
+
                        _db_release(snap->filters[snap->filter_cnt--]);
+
+                       dbs = realloc(snap->filters,
+                                     sizeof(struct db_filter *) * snap->filter_cnt);
+                       if (dbs != NULL)
+                               snap->filters = dbs;
                } while (snap->filter_cnt > col->filter_cnt);
        }

[coveralls]

coverage: 90.252% (+0.8%) from 89.454%
when pulling 893e70d on pcmoore:working-transactions
into 2847f10 on seccomp:main.

[drakenclimber]

I have a rough schedule the next few days, but I should have time at the end of the week to check this (and the other libseccomp patches) out later in the week.

[pcmoore]

I have a rough schedule the next few days, but I should have time at the end of the week to check this (and the other libseccomp patches) out later in the week.

Hi Tom, no immediate rush, but when you have the time it would be great to look over the other PRs; if you don't have time to merge them - assuming they look good to you - just leave your ACK and I'll take care of them.

As far as this PR is concerned, don't spend your time reviewing it just yet, I want to finish up a few things and make sure everything looks good on my end first; I'll leave a note when it is ready for review. Thanks for the previous review and feedback, it's been very helpful.

[drakenclimber]

I have a rough schedule the next few days, but I should have time at the end of the week to check this (and the other libseccomp patches) out later in the week.

Hi Tom, no immediate rush, but when you have the time it would be great to look over the other PRs; if you don't have time to merge them - assuming they look good to you - just leave your ACK and I'll take care of them.

As far as this PR is concerned, don't spend your time reviewing it just yet, I want to finish up a few things and make sure everything looks good on my end first; I'll leave a note when it is ready for review. Thanks for the previous review and feedback, it's been very helpful.

Sounds good. I'll focus on the other PRs this week. Let me know when this one is ready, and I'll start looking at it again.

[drakenclimber]

@pcmoore, I admit I haven't paid attention to this pull request lately. Do you want me to review, or are you still adding some more changes? Thanks!

[pcmoore]

I still need to give it a once over, but thanks for checking :)

[pcmoore]

Okay @drakenclimber, I think this is ready for a closer review when you have the time.

[drakenclimber]

The patchset looks great to me, @pcmoore. Thanks so much for this; I think it's a really good improvement.

[drakenclimber]

Merged to main. Thanks, @pcmoore.

One quick question for you, @pcmoore - Do you think we should cherrypick the first two patches into release-2.5? I don't have a strong feeling either way since I don't think this is a frequently-used feature by users.

[pcmoore]

I think we probably should do a backport of the two fixes. While yes, you are correct that users can't make direct use of the transaction capability in v2.5.Z, transactions are used internally and there is chance for a buggy abort operation if there is an error.

The backport should be pretty trivial, I see about doing that right now.