-
Notifications
You must be signed in to change notification settings - Fork 462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Misaligned address of _cups_sp_item_t struct #5474
Conversation
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.
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? |
@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. |
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. |
@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. |
@michaelrsweet The function _cupsStrRetain(...) has the following description: Note: This function does not verify that the passed pointer is in the 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. |
This set of changes makes the PPD functions use strdup and free - they were modifying the contents of the string in places and doing other things that were not safe for (immutable) strings in the pool.
This set of changes makes the PPD functions use strdup and free - they were modifying the contents of the string in places and doing other things that were not safe for (immutable) strings in the pool.
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) |
@ppawliczek I am doing a further review of callers to the string pool functions:
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. |
unaligned memory access (Issue #5474) Don't directly use the string pool in the CGI programs or scheduler.
…aligned memory access (Issue #5474) Don't directly use the string pool in the CGI programs or scheduler.
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) |
@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. |
@ppawliczek Any ETA on your next merge? |
Please let me know if the problem is not resolved... |
I do not know ETA for next CUPS merge. However, we will try to configure a public fuzzer for CUPS. |
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.