arm/filter_neon.S: remove obsolete assembler

[rossburton]

This file contains hand-coded assembler implementations of the filter functions for 32-bit Arm platforms. These are only used when the compiler doesn't support neon intrinsics (added to GCC 4.3 in 2008) or is exactly GCC 4.5.4 (released 2012), both of which are sufficiently unlikely to be true that it's fair to say the assembler is no longer used.

Instrinics can be optimised by the compiler, and also has the benefit that the compiler can add branch protection instructions (such as PAC/BTI on aarch64) to improve resilience against malicious input.

This commit deletes filter_neon.S and removes the now obsolete preprocessor logic in pngpriv.h.

Closes #505

[ggardet]

I think Makefile.in should be updated https://github.com/glennrp/libpng/blob/libpng16/Makefile.in#L111

[rossburton]

Yes, you're right. Pushed with the fix.

Generated files shouldn't be in git, really...

[rossburton]

We've discovered that building libpng without neon causes build failures and this patch is likely the cause:

In file included from ../libpng-1.6.40/arm/palette_neon_intrinsics.c:20:
recipe-sysroot-native/usr/lib/arm-poky-linux-gnueabi/gcc/arm-poky-linux-gnueabi/13.2.0/include/arm_neon.h: In function 'png_riffle_palette_neon':
recipe-sysroot-native/usr/lib/arm-poky-linux-gnueabi/gcc/arm-poky-linux-gnueabi/13.2.0/include/arm_neon.h:6791:1: error: inlining failed in call to 'always_inline' 'vdupq_n_u8': target specific option mismatch
  6791 | vdupq_n_u8 (uint8_t __a)
       | ^~~~~~~~~~
 ../libpng-1.6.40/arm/palette_neon_intrinsics.c:38:7: note: called from here

[ctruta]

Generated files shouldn't be in git, really...

I agree. But this is how I inherited the code base, and I didn't want to disrupt it, at least not in the 1.6.x series.
If you just run ./autogen.sh --maintainer, you should be good.

In any case, thank you very much for working on this. I am particularly happy to receive any contribution that consists in removing obsolete code.

[jbowler]

I think Makefile.in should be updated https://github.com/glennrp/libpng/blob/libpng16/Makefile.in#L111

It's a generated file; github is a distribution medium (it contains other configure generated files) so the Makefile.in change would automatically happen if the PR was accepted (but see below).

[jbowler]

I am particularly happy to receive any contribution that consists in removing obsolete code.

Off topic; this is why github discussions are useful :-)

@ctruta: Now you have cmake performing all the build steps you could remove all the configure stuff in libpng-1.8. There are lots of operating system distributions out there that mix cmake and configure projects but very few projects that have both build systems (and some that have neither; zlib).

All the makefiles in "scripts" could go too along with all the spurious unmaintained code in contrib which is not directly used in the build; for example intgamma.sh is just documentation of how the tables are produced, good for patent protection, bad for avoiding spurious bugs.

The documentation that is checked in, however, consists of generated files; the original documentation source isn't there so far as I can see (I never saw it.)

[billatarm]

For anyone watching this has been updated

[billatarm]

Can anyone mark this ready for review? Even though I can push to Ross's repo I can't twiddle the draft/ready state.

[rossburton]

Done, thanks Bill.

[jbowler]

I strongly suggest removing Makefile.in; the changes are incomplete as
revealed simply by typing autoreconf. It is important for security
reasons that changes to this file are only done by the maintainer with
the maintainer's supported version of autotools.

I can't vouch for the cmake changes but apart from Makefile.in the
configure changes seem to be correct.

I don't see a good reason for merging this into libpng 1.6. I do see a
good reason (simplification) for merging it into the next major release.

The png_riffle_palette_neon thing is apparently a separate bug - the
guard for that particular piece of code seems wrong. I think it should
be checking PNG_ARM_NEON_OPT like everything else.

For sure the old assembler should only ever get switched on in the two
very isolated cases @rossburton identifies. If these really are
"sufficiently unlikely to be true" there is no reason to take a merge in
1.6 which might break someone else; it works for those guys, so why
break it?

[billatarm]

I strongly suggest removing Makefile.in; the changes are incomplete as revealed simply by typing autoreconf.

I cannot replicate any failure, can you share how you're getting some error from autoreconf?

I see that now in the chat history that the autogenerated files are within the tree, odd. Looks like its all handled by "Rerun "./autogen.sh --maintainer" Is that ran by maintainers or should be included in the patch series?

It is important for security reasons that changes to this file are only done by the maintainer with the maintainer's supported version of autotools.

Can you elaborate why here? I maintain multiple autotools packages and have never encountered this.

I can't vouch for the cmake changes but apart from Makefile.in the configure changes seem to be correct.

I don't see a good reason for merging this into libpng 1.6. I do see a good reason (simplification) for merging it into the next major release.

It provides PAC and BTI support which are security features, so if libpng wants to ship with these architectural features enabled, that would be reason enough. I am assuming that 2.0 would be the next major release, is their a roadmap or plan? This in theory wouldn't cause any API breakage so not sure why it would wait.

The png_riffle_palette_neon thing is apparently a separate bug - the guard for that particular piece of code seems wrong. I think it should be checking PNG_ARM_NEON_OPT like everything else.

I can look into that more but that seems like a concern for a follow up change.

For sure the old assembler should only ever get switched on in the two very isolated cases @rossburton identifies. If these really are "sufficiently unlikely to be true" there is no reason to take a merge in 1.6 which might break someone else; it works for those guys, so why break it?

We can just fix it, I'll have to try and build without neon enabled. It might just be easier to add BTI/PAC support to that assembly code when PAC/BTI are enabled.

[jbowler]

It provides PAC and BTI support which are security features, so if libpng wants to ship with these architectural features enabled,

Eh? Where? So far as I can see it just deletes the assembler code; it does not ADD anything. If I'm wrong about this more review is required.

[jbowler]

We can just fix it, I'll have to try and build without neon enabled. It might just be easier to add BTI/PAC support to that assembly code when PAC/BTI are enabled.

You seem to be hijacking the PR. The PR is to remove "obsolete assembler" but you seem to have morphed it into a CVE. If there really is a security issue here I can fix it with just a one (EDIT: two) line change readily reversible by someone who doesn't give a thing about the security issue.

EDIT: #562

[billatarm]

It provides PAC and BTI support which are security features, so if libpng wants to ship with these architectural features enabled,

Eh? Where? So far as I can see it just deletes the assembler code; it does not ADD anything. If I'm wrong about this more review is required.

When you build with -mbranch-protection=standard all the generated object files have the BTI/PAC bit set in the gnu notes section of the ELF file, when linked together, the shared object will also be marked ad PAC/BTI support. However, with the assembler file, that gnu notes section is missing as it doesn;t instrument pac/bti support and the final link drops this from the resulting shared object. By deleting the assembly code and using only the intrinsics, the pac/bti support is properly added.

[billatarm]

We can just fix it, I'll have to try and build without neon enabled. It might just be easier to add BTI/PAC support to that assembly code when PAC/BTI are enabled.

You seem to be hijacking the PR. The PR is to remove "obsolete assembler" but you seem to have morphed it into a CVE.

I haven't hijacked anything, I am taking over for the original author. I have not morphed it into a CVE as their is known vulnerability. I am attempting to add a security feature. I think this additional benefit of supporting PAC/BTI is missed in the original commit message.

If there really is a security issue here I can fix it with just a one (EDIT: two) line change readily reversible by someone who doesn't give a thing about the security issue.

EDIT: #562

[jbowler]

When you build with -mbranch-protection=standard all the generated object files have the BTI/PAC bit set in the gnu notes section of the ELF file, when linked together, the shared object will also be marked ad PAC/BTI support. However, with the assembler file, that gnu notes section is missing as it doesn;t instrument pac/bti support and the final link drops this from the resulting shared object.

I think there's an "Issue" about this somewhere. It needs to be referenced in the PR.

IRC the issue is that the assembler file is included even if the intrinsics implementation is used and the assembler file generates an empty ELF with the relevant bits unset. Have I got this right? I distinctly remember this issue being discussed (I don't think this is déjà vu!)

[jbowler]

Ok, the original issue is #505 and it is the one referenced at the top by @rossburton

Back to what I said there: JUST remove filter_neon.S from Makefile.am and cmakelists.txt; leave all the logic and leave the file. Then we will find out if somehow someone is using it. It is possible for people to use it with non-broken compilers by explicitly defining PNG_ARM_NEON_IMPLEMENTATION but I regard that as "unsupported", so the only "supported" use is with the broken compilers, unless there is some issue with 32-bit?

[jbowler]

Here's another one:

#563

This does what I suggested. Tested with cmake and configure on amd64 and builds on aarch64 with configure --host=aarch64-linux-gnu. Needs a build test at least on arm32.

[jbowler]

We've discovered that building libpng without neon causes build failures and this patch is likely the cause:

I don't see how this is possible. The code flow in pngpriv.h only defines PNG_ARM_NEON_IMPLEMENTATION to a value other than 0 if PNG_ARM_NEON_OPT>0 and the code in both arm/*.c files explicitly check for PNG_ARM_NEON_IMPLEMENTATION==1.

Compare arm/palette_neon... with arm/filter_neon...; the latter is compiled first (based on the order in the Makefile) and appears to succeed. If I'm right that means PNG_ARM_NEON_OPT is 0 while PNG_ARM_NEON_IMPLEMENTATION is 1! So this problem looks like an unsupported set of compiler defines passed on the command line.

[billatarm]

Can someone close this, we went with #563 as the preferred approach.

[rossburton]

Closing as obsoleted by #563.

[ctruta]

Hello, everyone, and thank you for the fix #505.

I am reopening this PR, even if it is no longer applicable. You see, I liked the idea of removing filter_neon.S, because we already have filter_neon_intrinsics.c` already. I just returned to the libpng project, and I'm still catching up with my reading. Perhaps I haven't understood fully:

Is anybody against the idea of removing filter_neon.S completely?

[ctruta]

Closing again. See commit 9e53875, commit e4a31f0, and comment #505 (comment).

Many thanks for your contributions.