fuzzing harfbuzz #139
Comments
|
Next catch. echo AAEAAAADgCoAKvwNUEdEKkYAKeAAdAA9AAAAM0dQT1MA1UARAAAAAAoDAAMAAQAEAAgAAgADAAEABAABAA== | base64 --decode > reproducer |
|
Should be fixed. Please reopen with next issue :). |
|
Gooood! This has unblocked further fuzzing. Reproducer in base64: |
|
You should be able to reopen issues now. |
|
Leak fixed. |
|
Now, the bane of fuzzing: non-linear algorithms. For slow-unit-03b06616c9de07c85802e6a9a0652354266af8fd (takes 5 minutes to finish) (hm... github does not allow to upload .tgz file. The .pdf file attached is actually a tgz file. Weird) |
We probably should implement better system to catch cyclic lookups. But for now, this speeds up worst case behavior with broken fonts considerably without compromising legitimate usecases. #139 (comment)
|
I pushed out a change that makes these edge cases significantly faster (over 10x). We still can do better, but this should give you a boost for now. |
|
Did some tricks out of my sleeve... Note the "is located 127 bytes to the right of 1049-byte region" bit. Repro (base64) |
Discovered by libFuzzer. Ouch! #139 (comment)
|
one more case of a very slow run (23 seconds) |
|
The public bot is up and running (similar to the one for FreeType). Steps once the slowness is fixed:
|
|
The attached input takes over 400 seconds to process, all time is spent in |
Thanks. Will take a look. The problem is, the OpenType layout model is very expressive. You can easily get exponential expansion from it. That said, by handling circular recursions we might get most of those handled already... |
Overview of changes leading to 1.0.6 Thursday, October 15, 2015 ==================================== - Reduce max nesting level in OT lookups from 8 to 6. Should not affect any real font as far as I know. - Fix memory access issue in ot-font. - Revert default load-flags of fonts created using hb_ft_font_create() back to FT_LOAD_DEFAULT|FT_LOAD_NO_HINTING. This was changed in last release (1.0.5), but caused major issues, so revert. harfbuzz/harfbuzz#143 Overview of changes leading to 1.0.5 Tuesday, October 13, 2015 ==================================== - Fix multiple memory access bugs discovered using libFuzzer. harfbuzz/harfbuzz#139 Everyone should upgrade to this version as soon as possible. We now have continuous fuzzing set up, to avoid issues like these creeping in again. - Misc fixes. - New API: * hb_font_set_parent(). * hb_ft_font_[sg]et_load_flags() The default flags for fonts created using hb_ft_font_create() has changed to default to FT_LOAD_DEFAULT now. Previously it was defaulting to FT_LOAD_DFEAULT|FT_LOAD_NO_HINTING. - API changes: * Fonts now default to units-per-EM as their scale, instead of 0. * hb_font_create_sub_font() does NOT make parent font immutable anymore. hb_font_make_immutable() does.
To better understand this, see my new presentation, read the speaker notes, where I show that OpenType lookups are Turing complete if recursion depth is not limited... |
|
Is any workaround possible? Some way to limit the recursion (under a flag?) and return early? |
|
We already have a max recursion depth of 6 in place. I haven't had time to look at your worst examples to see what other measures I might be able to put in place. |
|
despite the slow runs, here is one more trophy. |
I still haven't studied the examples, but in the mean time I added a compile time way to limit the recursion depth even further. Just define HB_MAX_NESTING_LEVEL to 2 or 3. Setting it to 1 might be too limiting. |
Exactly the same problem that I fixed in 63ef0b4 I rewrote the table checking yesterday in 67f8821 and introduced the exact same issue again. :( Good thing we have ongoing fuzzing going now. Was discovered immediately by libFuzzer. Thanks kcc! #139 (comment) Fixes #156
So far I don't have any nice solution for cross-fuzzing two kinds of inputs. questions: |
Overview of changes leading to 1.0.6 Thursday, October 15, 2015 ==================================== - Reduce max nesting level in OT lookups from 8 to 6. Should not affect any real font as far as I know. - Fix memory access issue in ot-font. - Revert default load-flags of fonts created using hb_ft_font_create() back to FT_LOAD_DEFAULT|FT_LOAD_NO_HINTING. This was changed in last release (1.0.5), but caused major issues, so revert. harfbuzz/harfbuzz#143 Overview of changes leading to 1.0.5 Tuesday, October 13, 2015 ==================================== - Fix multiple memory access bugs discovered using libFuzzer. harfbuzz/harfbuzz#139 Everyone should upgrade to this version as soon as possible. We now have continuous fuzzing set up, to avoid issues like these creeping in again. - Misc fixes. - New API: * hb_font_set_parent(). * hb_ft_font_[sg]et_load_flags() The default flags for fonts created using hb_ft_font_create() has changed to default to FT_LOAD_DEFAULT now. Previously it was defaulting to FT_LOAD_DFEAULT|FT_LOAD_NO_HINTING. - API changes: * Fonts now default to units-per-EM as their scale, instead of 0. * hb_font_create_sub_font() does NOT make parent font immutable anymore. hb_font_make_immutable() does.
Less than 8 or 16 characters.
Doesn't have to be. The useful ones are. But HB should work regardless. |
|
What if we use hb_buffer_add_utf32 instead of utf8? |
Yes. It wouldn't exercise the UTF-8 decoder, but that's minor.
Yes, that works. And you'd need a new set of seeds I assume?
|
|
Let's do it then (treat last N*4 bytes of font as UTF32 string). |
|
We can use something like to fuzz utf32. |
Can you send a pull-request for that? Thanks. |
|
Maybe you could just apply the patch below? (Or I'll need some time to figure out how pull requests work...) |
|
Will do. Thanks. |
|
I also need to change the fuzzing code to print out the UTF-32 test, such that we can easily reproduce with hb-shape or copy into our test suite. |
Very rudimentary right now, but will get kcc's bot going. From #139 (comment)
Done. |
|
Have you seen this? I am getting it on 1.3.2, repro attached. (gzip-ed) |
As discovered by libFuzzer / Chromium fuzzing. Fixes https://bugs.chromium.org/p/chromium/issues/detail?id=659496 CC #139
Cannot reproduce using hb-fuzzer. Can you double-check? |
As discovered by libFuzzer / Chromium fuzzing. Fixes https://bugs.chromium.org/p/chromium/issues/detail?id=659496 CC harfbuzz#139
This has no security implications whatsoever since we always keep and extra element at the end of buffer, just in case. Discovered by oss-fuzz CC #139 Fixes https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=660
|
Self note for the next time:
|
For now it's not using a corpus since basically any string would work as a useful input and the input data is interpreted as UTF-32, then converted to UTF-16. This follow the discussion about raw input in the HarfBuzz fuzzing bug: harfbuzz/harfbuzz#139 (comment) Bug: 771594 Change-Id: Ibf60bb6b67c60131337a712c7b8baf6b5219a52b Reviewed-on: https://chromium-review.googlesource.com/700375 Commit-Queue: Dominik Röttsches <drott@chromium.org> Reviewed-by: Max Moroz <mmoroz@chromium.org> Cr-Commit-Position: refs/heads/master@{#507099}
For now it's not using a corpus since basically any string would work as a useful input and the input data is interpreted as UTF-32, then converted to UTF-16. This follow the discussion about raw input in the HarfBuzz fuzzing bug: harfbuzz/harfbuzz#139 (comment) Bug: 771594 Change-Id: Ibf60bb6b67c60131337a712c7b8baf6b5219a52b Reviewed-on: https://chromium-review.googlesource.com/700375 Commit-Queue: Dominik Röttsches <drott@chromium.org> Reviewed-by: Max Moroz <mmoroz@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#507099} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: b8cc3ad7241229a774e5182c62c4b1f0fc19ac95
Print proper cargo:include values
In order to build a Rust crate with C/C++ that uses the HarfBuzz library referenced by the `harfbuzz` crate, I need to know where the header files are. I believe the standard way of doing that is the `links` key/value environment variables. In particular, I'm expecting `DEP_HARFBUZZ_INCLUDE` to tell me the include path for `hb.h` and other header files. Unfortunately, this doesn't seem to be working correctly now. This PR is an attempt to resolve that.
Summary of changes:
* Prints proper values for `cargo:include` in the build script for both the `pkg-config` and vendored cases.
* Changes a use of `to_str().unwrap()` to `PathBuf::display()`. I believe this is better, but I'm happy to be corrected.
As far as adding a useful feature goes, I think this PR is complete. However, there are a couple of potential improvements that would be nice, but I'm not sure how to go about them.
**`not(feature = "build-native-harfbuzz")`**
I'm not exactly sure what disabling the feature `build-native-harfbuzz` does. It doesn't use `pkg-config` and doesn't build the vendored library? Consequently I'm not sure what to print for `cargo:include` here.
**Testing**
It would be nice to test for the presence and correctness of `DEP_HARFBUZZ_INCLUDE`. I used the following files (in the repository's top-level directory for convenience) both with `HARFBUZZ_SYS_NO_PKG_CONFIG=1` and without:
`Cargo.toml`:
```toml
[project]
name = "foo"
version = "0.5.0"
authors = []
build = "build.rs"
[dependencies]
harfbuzz-sys = { path = "harfbuzz-sys" }
[build-dependencies]
cc = "1.0"
```
`build.rs`:
```rust
use std::env;
fn main() {
let mut cfg = cc::Build::new();
for path in env::split_paths(&env::var_os("DEP_HARFBUZZ_INCLUDE").unwrap()) {
cfg.include(path);
}
cfg.file("src/test.c").warnings(false).compile("test");
}
```
`src/test.c`:
```c
#include <hb.h>
int test() {
hb_font_t *hb_font;
return 0;
}
```
`src/lib.rs`: empty
I'm not sure how to add such a test without simply writing a script and running it in CI. I did a quick search on GitHub and couldn't find any other projects that actually tested `cargo:include` in this way.
<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/139)
<!-- Reviewable:end -->
Add harfbuzz-sys-test using ctest This adds a crate to the workspace for testing `harfbuzz-sys` automatically using `ctest`. I changed a few types in `harfbuzz-sys/src/lib.rs` to make the testing automatic (i.e. to avoid creating special cases). However, I suppose `lib.rs` is generated? If so, this might be problematic if it is regenerated later. The changes seem quite sensible to me, so maybe there's an alternative to tweak the generation to get something similar? `harfbuzz-sys-test` currently only works with `HARFBUZZ_SYS_NO_PKG_CONFIG=1` because it doesn't get the `DEP_HARFBUZZ_INCLUDE` environment variable otherwise. The `.travis.yml` change reflects that. If harfbuzz#139 or something similar is merged, this special case can be removed, and the test should be run for both `HARFBUZZ_SYS_NO_PKG_CONFIG=1` and without `HARFBUZZ_SYS_NO_PKG_CONFIG`. There's a `FIXME` in `harfbuzz-sys-test/build.rs` because I don't understand the errors when these functions are not skipped. I would appreciate it if someone else would take a look at that. <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-harfbuzz/140) <!-- Reviewable:end -->
This in an umbrella issue for setting up regular fuzzing for harfbuzz and fixing the bugs that we find with fuzzing.
The starting point is the target function below used with libFuzzer.
Eventually we'll need to submit this function to harfbuzz repo and extend it to cover more code.
Currently, this is my workflow to build the fuzzer:
The text was updated successfully, but these errors were encountered: