Replace some assertions by AVIF_ASSERT() #1958

y-guyon · 2024-01-19T14:14:15Z

This is a general proposal, there are variants:

Use AVIF_CHECKERR() directly instead of introducing AVIF_ASSERT()
Introduce AVIF_RESULT_ASSERTION_FAILED instead of using AVIF_RESULT_UNKNOWN_ERROR

What do you think? If you agree with this initiative, it can be applied to more files in src/.

Example of why this PR is useful: AVIF_ASSERT(itemID != 0); in avifMetaFindOrCreateItem() could have sped the security investigation around #1922 up.

vrabaud · 2024-01-19T14:33:38Z

include/avif/internal.h

@@ -59,6 +59,17 @@ static inline void avifBreakOnError()
        }                                 \
    } while (0)

+// Same as AVIF_CHECKERR() but also assert() in debug build configuration.
+// Can be used instead of assert() for extra security.
+#define AVIF_ASSERT(A, ERR)     \


Not a big fan of the name. How about AVIF_ASSERT_AND_RETURN ?

Why not AVIF_ASSERT_OR_RETURN indeed

wantehchang · 2024-01-19T23:21:42Z

src/read.c

@@ -566,7 +566,7 @@ static avifResult avifCodecDecodeInputFillFromDecoderItem(avifCodecDecodeInput *
            }
        }
        if (remainingSize > 0) {
-            assert(layerCount == 3);
+            AVIF_ASSERT(layerCount == 3, AVIF_RESULT_UNKNOWN_ERROR);


We should only use assert() for conditions that must be true by logical reasoning. We should not use assert() to detect bad inputs or environment errors (e.g., filesystem errors). Under this policy, an assertion failure indicates a bug in our code. If we want to return an error in that case, let's add a new error code AVIF_RESULT_INTERNAL_ERROR.

Another issue is that some assertion failures imply that memory may have been corrupted or our data structures may be in an inconsistent state, so it may be unsafe to return an error and continue execution. This is why in Google's internal repository and in Chrome, there is a CHECK that is always enabled. We may want to consider adding our equivalent of CHECK. It would be simpler than analyzing whether it is safe to return an error and continue execution.

More on my suggestion of adding AVIF_RESULT_INTERNAL_ERROR:

AVIF_RESULT_UNKNOWN_ERROR is equivalent to absl::StatusCode::kUnknown, defined as follows:

// kUnknown [...] indicates an unknown error occurred. In // general, more specific errors should be raised, if possible. Errors raised // by APIs that do not return enough error information may be converted to // this error.

AVIF_RESULT_INTERNAL_ERROR is equivalent to absl::StatusCode::kInternal, defined as follows:

// kInternal [...] indicates an internal error has occurred // and some invariants expected by the underlying system have not been // satisfied. This error code is reserved for serious errors.

We should only use assert() for conditions that must be true by logical reasoning. We should not use assert() to detect bad inputs or environment errors (e.g., filesystem errors). Under this policy, an assertion failure indicates a bug in our code. If we want to return an error in that case, let's add a new error code AVIF_RESULT_INTERNAL_ERROR.

Done with AVIF_CHECKERR(..., AVIF_RESULT_INTERNAL_ERROR).
Thank you for checking abseil status code meanings.

Another issue is that some assertion failures imply that memory may have been corrupted or our data structures may be in an inconsistent state, so it may be unsafe to return an error and continue execution. This is why in Google's internal repository and in Chrome, there is a CHECK that is always enabled. We may want to consider adding our equivalent of CHECK. It would be simpler than analyzing whether it is safe to return an error and continue execution.

Terminating execution may be safer but crashes are inconvenient for users.

In the example mentioned in the PR description, I believe libavif refusing to decode a few rare images (color grid but no alpha grid) is better than crashing the AVIF decoding, potentially also the whole service using libavif as a dependency. Most of the checks enforced in this PR are preventive ("alpha should be defined before being used" etc). Returning an error there seems fine.

I suggest replacing most assertions in libavif by AVIF_CHECKERR(..., AVIF_RESULT_INTERNAL_ERROR) and having some kind of CHECK in critical places, if necessary.

Note: I just read the message as the beginning of this pull request. Your AVIF_RESULT_ASSERTION_FAILED is essentially the AVIF_RESULT_INTERNAL_ERROR I suggested.

wantehchang

LGTM. I suggest some tweaks of the comment and error message for AVIF_RESULT_INTERNAL_ERROR.

I also think that the conditions that can be easily shown to be true can be safely checked by assert(), but I wasn't able to inspect all the changes in this pull request. Note: Using assert() to check such conditions is just a code size optimization, and the effort to prove a condition is true may depend on familiarity with the code or knowledge of the underlying specs, so "easily shown to be true" may not be absolute.

wantehchang · 2024-01-23T15:33:22Z

src/avif.c

@@ -103,6 +103,7 @@ const char * avifResultToString(avifResult result)
        case AVIF_RESULT_OUT_OF_MEMORY:                 return "Out of memory";
        case AVIF_RESULT_CANNOT_CHANGE_SETTING:         return "Cannot change some setting during encoding";
        case AVIF_RESULT_INCOMPATIBLE_IMAGE:            return "The image is incompatible with already encoded images";
+        case AVIF_RESULT_INTERNAL_ERROR:                return "Some invariants have not been satisfied";


The error message can just say "Internal error".

wantehchang · 2024-01-23T15:35:54Z

include/avif/avif.h

@@ -186,10 +186,11 @@ typedef enum AVIF_NODISCARD avifResult
    AVIF_RESULT_OUT_OF_MEMORY = 26,
    AVIF_RESULT_CANNOT_CHANGE_SETTING = 27, // a setting that can't change is changed during encoding
    AVIF_RESULT_INCOMPATIBLE_IMAGE = 28,    // the image is incompatible with already encoded images
+    AVIF_RESULT_INTERNAL_ERROR = 29,        // some invariants have not been satisfied


Nit: I suggest we have a short primary definition "internal error", followed by an explanation that say "some invariants have been satisfied". Ideally the definition should make it clear that this error indicates a bug in libavif.

wantehchang · 2024-01-23T15:50:30Z

src/read.c

@@ -566,7 +566,7 @@ static avifResult avifCodecDecodeInputFillFromDecoderItem(avifCodecDecodeInput *
            }
        }
        if (remainingSize > 0) {
-            assert(layerCount == 3);
+            AVIF_CHECKERR(layerCount == 3, AVIF_RESULT_INTERNAL_ERROR);


I only inspected this change, and this is an example of condition that can be safely checked with assert(). The reason is that one can prove this condition is true by analyzing only a few lines of code (specifically, line 544 and lines 551-568).

It seems that AVIF_CHECKERR(..., AVIF_RESULT_INTERNAL_ERROR) is more appropriate for conditions that can only be proved true by analyzing multiple functions or complicated code.

wantehchang

Yannis: I just realized that I made a mistake in the decision to approve this pull request.

Influenced by the AVIF_ASSERT() in the title of this pull request, I somehow thought the AVIF_CHECKERR(condition, AVIF_RESULT_INTERNAL_ERROR) in this pull request would be backed by an assert() in Debug build, so that this pull request would be essentially a no-op in Debug build and a win (because of the error handling) in Release build.

But this pull request replaces assert() with error handling in both Debug and Release builds. As a debugging aid, this is a downgrade in Debug build because we won't get an error message (emitted by assert()) right when a bug is detected. If we can reproduce the bug, this is just an inconvenience. But if we can't reproduce the bug, this is a loss of useful debugging information.

So it may be better to convert fewer assertions so that we can inspect them. For example, the assert(itemID != 0) in avifMetaFindOrCreateItem() can be converted.

y-guyon · 2024-01-24T09:44:57Z

Influenced by the AVIF_ASSERT() in the title of this pull request, I somehow thought the AVIF_CHECKERR(condition, AVIF_RESULT_INTERNAL_ERROR) in this pull request would be backed by an assert() in Debug build, so that this pull request would be essentially a no-op in Debug build and a win (because of the error handling) in Release build.

But this pull request replaces assert() with error handling in both Debug and Release builds. As a debugging aid, this is a downgrade in Debug build because we won't get an error message (emitted by assert()) right when a bug is detected. If we can reproduce the bug, this is just an inconvenience. But if we can't reproduce the bug, this is a loss of useful debugging information.

In my opinion this is easily caught by using avifBreakOnError(). Currently I just add a ! in #if defined(AVIF_BREAK_ON_ERROR), it is quite convenient. It also catches other errors when reproducing.

wantehchang · 2024-01-25T01:41:32Z

What I meant is that if we can't reproduce the bug, printing an error message and aborting the process immediately when a bug is detected is more helpful to debugging than returning AVIF_RESULT_INTERNAL_ERROR.

In any case, all options for this issue have pros and cons, so it is fine to try this approach.

In my experience, people have a hard time agreeing on how assertions should be used. So I think it is good to adopt a policy or guidelines for assertions from a reputable software organization (similar to style guides for programming languages). I suggest we adopt Chrome's policy for assertions (CHECK , DCHECK, etc.) and gradually move toward it:
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md

wantehchang · 2024-01-25T02:42:34Z

A complication is that an AVIF_RESULT_INTERNAL_ERROR return value will need to be treated as a bug. If a test allows some function to fail with any error (e.g., a fuzz test may allow a decoding function to fail with any error), that test will need to disallow that function to fail with AVIF_RESULT_INTERNAL_ERROR.

y-guyon · 2024-01-25T06:26:04Z

What I meant is that if we can't reproduce the bug, printing an error message and aborting the process immediately when a bug is detected is more helpful to debugging than returning AVIF_RESULT_INTERNAL_ERROR.

Ah, got it. Good point. This is also true for any returned error, not only the current assertions. So overall logging could be improved in libavif.

I suggest we adopt Chrome's policy for assertions (CHECK , DCHECK, etc.) and gradually move toward it:
https://chromium.googlesource.com/chromium/src/+/main/styleguide/c++/checks.md

I am still uneasy with crashing as a default behavior. It may be fine in a Chromium environment but annoying for other users of libavif.

A complication is that an AVIF_RESULT_INTERNAL_ERROR return value will need to be treated as a bug. If a test allows some function to fail with any error (e.g., a fuzz test may allow a decoding function to fail with any error), that test will need to disallow that function to fail with AVIF_RESULT_INTERNAL_ERROR.

Indeed.

I suggest this PR to introduce AVIF_ASSERT_OR_RETURN() to keep the debug assertions, with the current changes to read.c and write.c.
Then we can decide later whether to keep it as is, to replace it by some AVIF_CHECK_OR_ABORT() or to introduce a CMake flag to switch between the two behaviors.
@wantehchang Would that be ok? My intent was really to have safer checks in release as early as possible, and all of the variants here go in that direction anyway.

wantehchang · 2024-01-26T00:33:46Z

Yannis: Yes, your suggestion of adding AVIF_ASSERT_OR_RETURN() is ok. I like that better than the current approach. At least we won't need to update the fuzz tests to check for AVIF_RESULT_INTERNAL_ERROR. (I think we build our fuzz tests with assertions enabled.)

include/avif/internal.h

wantehchang · 2024-01-26T23:45:46Z

include/avif/internal.h

@@ -59,6 +59,17 @@ static inline void avifBreakOnError()
        }                                 \
    } while (0)

+// Same as AVIF_CHECKERR() but also assert() in debug build configuration.
+// Can be used instead of assert() for extra security in release builds.
+#define AVIF_ASSERT_OR_RETURN(A, ERR) \


Delete the ERR parameter. This macro should always use AVIF_RESULT_INTERNAL_ERROR.

This macro should always use AVIF_RESULT_INTERNAL_ERROR.

I agree but at calling site it is not as readable in my opinion. What about a static_assert() to make sure ERR is always AVIF_RESULT_INTERNAL_ERROR?

I see what you mean. Here is an example:

AVIF_ASSERT_OR_RETURN(layerCount == 3)

I think this is caused by the macro name. Perhaps we can rename the macro AVIF_ASSERT_OR_RETURN_ERROR or even AVIF_ASSERT_OR_RETURN_INTERNAL_ERROR.

Adding the ERR parameter back is also fine by me.

static_assert is only available in C11 or later, so we will need to add an AVIF_STATIC_ASSERT macro if we want to use static_assert in our C code.

wantehchang · 2024-01-29T19:13:42Z

include/avif/internal.h

+#define AVIF_ASSERT_OR_RETURN(A) AVIF_CHECKERR((A), AVIF_RESULT_INTERNAL_ERROR)
+#else
+#define AVIF_ASSERT_OR_RETURN(A) assert(A)
+#endif


[Informational, not a request for change.] Please see ABSL_HARDENING_ASSERT. It seems to be Abseil's equivalent of CHECK.

[skip ci]

vrabaud approved these changes Jan 19, 2024

View reviewed changes

wantehchang reviewed Jan 19, 2024

View reviewed changes

y-guyon requested a review from wantehchang January 22, 2024 23:16

wantehchang approved these changes Jan 23, 2024

View reviewed changes

wantehchang reviewed Jan 23, 2024

View reviewed changes

y-guyon requested a review from wantehchang January 26, 2024 14:48

wantehchang reviewed Jan 26, 2024

View reviewed changes

wantehchang approved these changes Jan 27, 2024

View reviewed changes

wantehchang reviewed Jan 29, 2024

View reviewed changes

y-guyon added 14 commits January 30, 2024 10:51

Replace some assertions by AVIF_ASSERT()

4eb7bbc

Replace AVIF_ASSERT(UNKNOWN) by CHECKERR(INTERNAL)

312863b

avifResult avifDecoderDataCopyTileToImage()

ff5936d

AVIF_CHECKERR in avifDecoderDecodedRowCount()

e7557ac

avifResult avifImageCopyAndPad()

e53c4c6

Apply clang-format

0841eb3

Fix avifImageCopyAndPad()

a4535a4

Add CHANGELOG.md entry

c1f0963

[skip ci]

Revert aviflosslesstest.cc clang-format

a40d85a

[skip ci]

Revert clang-format

357341b

[skip ci]

Revert more clang-format

7aad9f5

[skip ci]

Add AVIF_ASSERT_OR_RETURN()

1feebcf

Update comments

ffa034d

[skip ci]

Change AVIF_ASSERT_OR_RETURN() definition

1b4ef3a

y-guyon force-pushed the avif_assert branch from 37361e3 to 1b4ef3a Compare January 30, 2024 09:52

y-guyon merged commit b4e5d98 into AOMediaCodec:main Jan 30, 2024
20 checks passed

y-guyon deleted the avif_assert branch January 30, 2024 10:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace some assertions by AVIF_ASSERT() #1958

Replace some assertions by AVIF_ASSERT() #1958

y-guyon commented Jan 19, 2024

vrabaud Jan 19, 2024

y-guyon Jan 19, 2024

wantehchang Jan 19, 2024

wantehchang Jan 21, 2024

y-guyon Jan 22, 2024

wantehchang Jan 23, 2024

wantehchang left a comment •

edited

Loading

wantehchang Jan 23, 2024

wantehchang Jan 23, 2024

wantehchang Jan 23, 2024

wantehchang left a comment

y-guyon commented Jan 24, 2024

wantehchang commented Jan 25, 2024

wantehchang commented Jan 25, 2024

y-guyon commented Jan 25, 2024

wantehchang commented Jan 26, 2024

wantehchang Jan 26, 2024

y-guyon Jan 29, 2024

wantehchang Jan 29, 2024

wantehchang Jan 29, 2024

Replace some assertions by AVIF_ASSERT() #1958

Replace some assertions by AVIF_ASSERT() #1958

Conversation

y-guyon commented Jan 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wantehchang left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wantehchang left a comment

Choose a reason for hiding this comment

y-guyon commented Jan 24, 2024

wantehchang commented Jan 25, 2024

wantehchang commented Jan 25, 2024

y-guyon commented Jan 25, 2024

wantehchang commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wantehchang left a comment •

edited

Loading