Misaligned address of _cups_sp_item_t struct

[ppawliczek]

Strings registered with functions declared in cups/string.h were sometimes
converted in-place to _cups_sp_item_t structures. This caused problem with
misaligned addresses of temporary _cups_sp_item_t structures created this way.
This patch introduced a couple of modifications to string.c, now all used
_cups_sp_item_t structures are created and filled in proper way.

[michaelrsweet]

No. Your change allocates the search key every time we look up a string, which will do the exact opposite of what the string pool code is intended to do (efficiently store immutable copies of strings)

We have unit tests for this code and have no reports of problems - where are you seeing issues, and what are the backtraces?

[michaelrsweet]

@ppawliczek I've done some more investigation - seems like configuring the CUPS source with the "--enable-debug-guards" option should help you find where the string pool corruption is happening. To reiterate my previous comment, this code needs to run fast and allocating and copying every time is not compatible with that goal (otherwise we'd just allocate separate copies of the string every time...) - we are trading string comparison time for dramatically-reduced memory usage.

[ppawliczek]

Hi Michael! I am sorry for late answer. First of all, let me say that I completely understand your point of view. However dereferencing an unaligned struct is an undefined behavior in C. Since we work with several architectures we decided to patch CUPS on our side.

[michaelrsweet]

@ppawliczek The first member of the struct is a 32-bit integer. Short of a bug, all C string constants should be at least 32-bit aligned on modern platforms (I know macOS uses 64-bit alignment for performance), so I'd really appreciate some additional information (including the platform you are running on) to determine what is going wrong. Your change introduces a significant performance regression and doesn't address usage of _cupsStrRetain.

[ppawliczek]

@michaelrsweet The function _cupsStrRetain(...) has the following description:
/*
'_cupsStrRetain()' - Increment the reference count of a string.

Note: This function does not verify that the passed pointer is in the
string pool, so any calls to it MUST know they are passing in a
good pointer.
*/
It is always given a correct pointer, so there is no reason to make any changes there.

The problem with patched functions is that sometimes the given pointer (char*) is not aligned and this results in unaligned pointer to struct _cups_sp_item_t. It was detected by our sanitizer (dereferencing unaligned structure) when parsing PPD file.
I know this change hits overall performance. However our tests show no measurable difference between patched and unpatched version. It is probably caused by the fact that vast majority of CPU time is spent in various external filters (pdftopdf, ghostscript etc), not in CUPS.

[michaelrsweet]

OK, my first pass is to change the PPD API to not use the string pool, since it was modifying the strings after allocation and, as you noted, passing some unaligned strings:

[master 5118c5739] Fix potential unaligned accesses in the string pool (Issue #5474)

[branch-2.2 6844678] Fix potential unaligned accesses in the string pool (Issue #5474)

[michaelrsweet]

@ppawliczek I am doing a further review of callers to the string pool functions:

• _cupsStrAlloc should not care about alignment since we never access the ref_count member during the search
• _cupsStrFree validates the pointer before accessing ref_count - this should likewise not need any special handling (except when debug guards are enabled, but that isn't something that is used in production... I could change the code there slightly to search first and then check, however)
• _cupsStrRetain is supposed to only be used on strings from the pool as an optimization - I will audit all calls to ensure they are always used correctly.

I would appreciate your testing current Git master (without your changes) to let me know whether you are still able to reproduce the issues you originally reported.

[michaelrsweet]

Found some other usage of the string pool outside CUPS proper and removed it. Also switched to _cupsStrAlloc where it wasn't guaranteed that the string was in the pool.

Finally, updated _cupsStrFree to look for the guard after a match.

[master 86c184f] Clean out some more _cupsStr cruft that might potentially cause an unaligned memory access (Issue #5474)

[branch-2.2 ddcb034] Clean out some more _cupsStr cruft that might potentially cause an unaligned memory access (Issue #5474)

[ppawliczek]

@michaelrsweet Unfortunately, I am not able to test the current master. I have no control over our testing infrastructure, it runs test on our "master" only. So, I will have some results when we merge new CUPS version to our repository.

[michaelrsweet]

@ppawliczek Any ETA on your next merge?

[michaelrsweet]

Please let me know if the problem is not resolved...

[ppawliczek]

I do not know ETA for next CUPS merge. However, we will try to configure a public fuzzer for CUPS.