Skip to content

cmake: don't disable _FORTIFY_SOURCE - #5227

Open
jzern wants to merge 1 commit into
AOMediaCodec:mainfrom
jzern:fortify-source
Open

cmake: don't disable _FORTIFY_SOURCE#5227
jzern wants to merge 1 commit into
AOMediaCodec:mainfrom
jzern:fortify-source

Conversation

@jzern

@jzern jzern commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

This was inherited from libaom, which inherited it from libvpx, where it
was added long ago as an attempt for compatibility across glibc
versions. Formalizing it in the build was never a great idea as this
provides some security benefits. Projects like Gentoo have removed this
from the build flags for some time.

CFLAGS/AVM_EXTRA_C_FLAGS can still be used to set these values if
needed.

This matches the changes in libvpx and libaom:
368c5f89d configure: don't disable _FORTIFY_SOURCE
ec0dedc1a2 cmake: don't disable _FORTIFY_SOURCE

This was inherited from libaom, which inherited it from libvpx, where it
was added long ago as an attempt for compatibility across glibc
versions. Formalizing it in the build was never a great idea as this
provides some security benefits. Projects like Gentoo have removed this
from the build flags for some time.

CFLAGS/AVM_EXTRA_C_FLAGS can still be used to set these values if
needed.

This matches the changes in libvpx and libaom:
 368c5f89d configure: don't disable _FORTIFY_SOURCE
 ec0dedc1a2 cmake: don't disable _FORTIFY_SOURCE
@jzern
jzern marked this pull request as ready for review August 6, 2026 23:41
@jzern
jzern requested a review from wantehchang August 6, 2026 23:41
@jzern

jzern commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

The build failure looks like a preexisting condition:

../avm_dsp/grain_table.c: In function 'grain_table_entry_read':
../avm_dsp/grain_table.c:131:11: error: ignoring return value of 'fscanf' declared with attribute 'warn_unused_result' [-Werror=unused-result]
  131 |     (void)fscanf(file, "\n");
      |

I made some updates to this file in libaom recently to clear some clang-19 static analysis warnings, but I didn't get to this particular call.

@wantehchang

Copy link
Copy Markdown
Member

James: I remember your CL for the aom_dsp/grain_table.c file in libaom and we discussed that (void)fscanf(file, "\n") call.

@jzern

jzern commented Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

James: I remember your CL for the aom_dsp/grain_table.c file in libaom and we discussed that (void)fscanf(file, "\n") call.

You're right. The comments have the detail about the last remaining issues in this file. From what I remember more checks than were necessary were needed to satisfy the static analyzer, but I didn't keep the patch around.

aom_dsp/grain_table.c:287:15: warning: File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior [unix.Stream]
  287 |       int c = fgetc(file);
      |               ^~~~~~~~~~~
aom_dsp/grain_table.c:287:15: warning: Read function called when stream is in EOF state. Function has no effect [unix.Stream]
  287 |       int c = fgetc(file);
      |               ^~~~~~~~~~~
aom_dsp/grain_table.c:291:7: warning: File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior [unix.Stream]
  291 |       ungetc(c, file);
      |       ^~~~~~~~~~~~~~~

The html report for those errors is in the libaom static analysis job:

Note the last two links may be unstable.

@urvangjoshi

Copy link
Copy Markdown
Contributor

James: I remember your CL for the aom_dsp/grain_table.c file in libaom and we discussed that (void)fscanf(file, "\n") call.

You're right. The comments have the detail about the last remaining issues in this file. From what I remember more checks than were necessary were needed to satisfy the static analyzer, but I didn't keep the patch around.

aom_dsp/grain_table.c:287:15: warning: File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior [unix.Stream]
  287 |       int c = fgetc(file);
      |               ^~~~~~~~~~~
aom_dsp/grain_table.c:287:15: warning: Read function called when stream is in EOF state. Function has no effect [unix.Stream]
  287 |       int c = fgetc(file);
      |               ^~~~~~~~~~~
aom_dsp/grain_table.c:291:7: warning: File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior [unix.Stream]
  291 |       ungetc(c, file);
      |       ^~~~~~~~~~~~~~~

The html report for those errors is in the libaom static analysis job:

Note the last two links may be unstable.

What do we need to do to unblock this PR?

@jzern

jzern commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

James: I remember your CL for the aom_dsp/grain_table.c file in libaom and we discussed that (void)fscanf(file, "\n") call.

You're right. The comments have the detail about the last remaining issues in this file. From what I remember more checks than were necessary were needed to satisfy the static analyzer, but I didn't keep the patch around.

aom_dsp/grain_table.c:287:15: warning: File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior [unix.Stream]
  287 |       int c = fgetc(file);
      |               ^~~~~~~~~~~
aom_dsp/grain_table.c:287:15: warning: Read function called when stream is in EOF state. Function has no effect [unix.Stream]
  287 |       int c = fgetc(file);
      |               ^~~~~~~~~~~
aom_dsp/grain_table.c:291:7: warning: File position of the stream might be 'indeterminate' after a failed operation. Can cause undefined behavior [unix.Stream]
  291 |       ungetc(c, file);
      |       ^~~~~~~~~~~~~~~

The html report for those errors is in the libaom static analysis job:

Note the last two links may be unstable.

What do we need to do to unblock this PR?

As quick workaround would be to disable -Werror=unused-result for this file. I was assuming that there would be similar failures in other PRs, but maybe this change was enough to uncover the warning. Wan-Teh mentioned the CL in libaom where we discussed this line; that change also had some nice to have static analysis warning reductions.

The fix for the warning should just be to check the error return and decide if this newline is optional or not. From what I remember, that wasn't sufficient for the static analyzer given the loop structure.

@jzern

jzern commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

I haven't been able to reproduce the bot failure with gcc 15.2.0 nor gcc 14.3.0 (cmake 4.2.3) locally. But the fix is straightforward. I'll deal with this in libaom and then try to port the fix (and others) to this copy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants