Add lzo #1862

bshastry · 2018-10-08T15:27:34Z

This PR:

Adds the lzo library to oss-fuzz (CC Potential new projects to integrate with OSS-Fuzz #402)
Adds target out-of-source to seek feedback from maintainer @markus-oberhumer

jonathanmetzman

We approve of adding this project.
Did you want feedback from @markus-oberhumer before we merged this?

jonathanmetzman · 2018-10-08T16:24:07Z

projects/lzo/lzo_compress_target.c

+
+    /* Decompress. */
+    new_len = size;
+    r = lzo1x_decompress(out,out_len,in,&new_len,NULL);


I think it would be nice to have another target that tried to decompress data. Right now lzo1x_decompress is not decompressing anything invalid.

I agree. @markus-oberhumer What do you think of adding lzo to oss fuzz?

Would it be better to move this and future test harnesses to lzo repo? Currently this PR picks up an lzo release. Would it be better to fuzz the current development commit instead? I couldn't find a public repository for lzo unfortunately.

Markus: we prefer to have fuzz targets upstream (less likely to break)

@jonathanmetzman I added a decompress only target in the source branch of this PR. Looking forward to feedback.

At the moment, there is a shallow heap-buffer overrun (READ) like so for a 1 byte compressed data:

READ of size 8 at 0x6020000000d1 thread T0 SCARINESS: 23 (8-byte-read-heap-buffer-overflow) #0 0x5bdcce in lzo1x_decompress /src/lzo-2.10/src/lzo1x_d.ch:120:13

bshastry · 2018-10-08T17:42:54Z

Brainstorming here: To do this systematically, we would need to add two targets per compression method, one that compresses fuzzed data and another that decompresses it. I believe there are 10 or more compression methods in the current release version, do that would mean around 20 targets.

This is excluding other compression parameters such as the number of rounds to use etc.

Wouldn't having so many targets sit in the oss-fuzz repo lead to bit rot?

jonathanmetzman · 2018-10-08T18:05:56Z

Brainstorming here: To do this systematically, we would need to add two targets per compression method, one that compresses fuzzed data and another that decompresses it.

I think having these two kinds of targets are worthwhile:

decompress(compress(data))

and

decompress(data)

(decompression in the first target is controversial, since theoretically it shouldn't cover anything the second target won't, but my guess is it would be useful in practice).

I believe there are 10 or more compression methods in the current release version, do that would mean around 20 targets.

This is excluding other compression parameters such as the number of rounds to use etc.

So instead of creating one (or two) new target(s) for every method/parameter combination, you can
take some bytes from the start of data to determine which compression method to use and what the parameters will be. Sort of like this:

index = data[0] % compression_methods.length;
method = compression_methods[index]
method(data[1], size-1)

Wouldn't having so many targets sit in the oss-fuzz repo lead to bit rot?

Yes. This is probably too many.

bshastry · 2018-10-08T18:33:37Z

Thank you @jonathanmetzman for the discussion.

I will address these concerns on my fork and ping you back when that's ready for review.

In the meanwhile, looking forward to hearing from Markus.

bshastry · 2018-10-10T11:39:35Z

Ping back from Berlin :-)

@markus-oberhumer: What email address should I use as the primary contact in project.yaml? At the moment, it is "info at oberhumer.com" but this looks like a sales related contact end-point.

This commit adds a minimal lzo seed as a seed for the lzo_decompress_target. Still results in a heap-buffer-overflow at the moment.

…d remove assertion

googlebot · 2018-10-10T19:59:57Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

bshastry · 2018-10-10T20:03:16Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

@viniul I think you need to sign the Google CLA :-)

viniul · 2018-10-10T22:15:23Z

I'll make sure to sign it :)

This commit adds more decompression targets to decompress_target.c. The target function is chosen based on the first byte of the data given by libfuzzer.

…include paths in build script

jonathanmetzman · 2018-10-18T15:52:17Z

ping.
@viniul are you still planning on signing the cla?

viniul · 2018-10-18T16:24:37Z

@jonathanmetzman Sorry, I did not have the time to look into it yet. Planning on signing it this weekend.

viniul · 2018-10-19T11:21:51Z

@jonathanmetzman I have filled out and submitted the Google CLA.

jonathanmetzman · 2018-10-19T15:41:56Z

In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@bshastry can you comment again so that I can see if the bot updates the status?
Some of the docs imply that the status won't be updated because the pull request has multiple authors, but I guess we will find out.

bshastry · 2018-10-19T15:51:27Z

Bot do you think?

Dor1s · 2018-10-19T18:02:58Z

Yeah, we need to make sure that CLA is signed for each email from the following:

$ git log -n 13 | egrep -i author | uniq
Author: Bhargava Shastry <bshastry@sect.tu-berlin.de>
Author: Vincent Ulitzsch <vincent.ulitzsch@live.de>
Author: Vincent Ulitzsch <vincent@srlabs.de>
Author: Bhargava Shastry <bshastry@sect.tu-berlin.de>
Author: Vincent Ulitzsch <vincent@srlabs.de>
Author: Bhargava Shastry <bshastry@sect.tu-berlin.de>

or you can try to squash the commits and upload a separate PR / force update this one.

viniul · 2018-10-21T16:11:52Z

Hey @Dor1s - I signed the Google CLA for the emails I used for the commits. I think @bshastry signed his already - so we should be good to go. Can you confirm?

bshastry · 2018-10-21T16:26:36Z

Just a check on the cla.

googlebot · 2018-10-21T16:26:41Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

Dor1s · 2018-10-21T16:59:04Z

@viniul @bshastry thank you! Now I have to figure out how to set that cla label to yes.

Dor1s · 2018-10-23T14:22:33Z

Ok, as per some documentation I've found, @googlebot is not going to change the status here, and we can merge after getting the consent from all authors. Given that we have only two authors here (with a couple different git emails), I think we have a consent from everyone. Merging.

This reverts commit 754db9e.

bshastry added 2 commits October 8, 2018 17:15

lzo: Add lzo out-of-source

03ca784

lzo: Change maintainer of Docker image

2fb96b3

jonathanmetzman requested changes Oct 8, 2018

View reviewed changes

bshastry and others added 7 commits October 10, 2018 14:22

lzo: Add decompress target

74fb88a

lzo: Update build script

6b9c79e

lzo: Fix build script

e0f387a

lzo: Bail out if size==0 in decompress test

d563bbf

Adding a seed file to lzo_decompress_target_seed

665ac86

This commit adds a minimal lzo seed as a seed for the lzo_decompress_target. Still results in a heap-buffer-overflow at the moment.

lzo: Switch to safer decompressor (HT @viniul) that does not crash an…

39db92a

…d remove assertion

Merge #1 by @viniul into origin_add_lzo

c04c6fd

lzo: Remove buggy addition (from OvS) to this PR

d014354

Vincent Ulitzsch and others added 3 commits October 11, 2018 01:02

Add more decompression targets to decompress_target

da5ea52

This commit adds more decompression targets to decompress_target.c. The target function is chosen based on the first byte of the data given by libfuzzer.

Merge branch 'add-lzo' into add-lzo

9e08efa

Make decomp func ptr static, fix minor bug in func ptr init, and fix …

e4a51ca

…include paths in build script

Dor1s merged commit 754db9e into google:master Oct 23, 2018

inferno-chromium added a commit that referenced this pull request Oct 23, 2018

Revert "[lzo] Add lzo (#1862)"

8d51afd

This reverts commit 754db9e.

inferno-chromium mentioned this pull request Oct 23, 2018

Revert "Add lzo" #1900

Merged

inferno-chromium added a commit that referenced this pull request Oct 23, 2018

Revert "[lzo] Add lzo (#1862)" (#1900)

cf78fed

This reverts commit 754db9e.

This was referenced Oct 25, 2018

Add lzo #1904

Closed

lzo: Fixes problems with google/oss-fuzz#1900 #1905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add lzo #1862

Add lzo #1862

bshastry commented Oct 8, 2018 •

edited

Loading

jonathanmetzman left a comment

jonathanmetzman Oct 8, 2018

bshastry Oct 8, 2018

jonathanmetzman Oct 8, 2018

bshastry Oct 10, 2018 •

edited

Loading

bshastry commented Oct 8, 2018

jonathanmetzman commented Oct 8, 2018

bshastry commented Oct 8, 2018

bshastry commented Oct 10, 2018 •

edited

Loading

googlebot commented Oct 10, 2018

bshastry commented Oct 10, 2018

viniul commented Oct 10, 2018

jonathanmetzman commented Oct 18, 2018

viniul commented Oct 18, 2018

viniul commented Oct 19, 2018

jonathanmetzman commented Oct 19, 2018

bshastry commented Oct 19, 2018

Dor1s commented Oct 19, 2018

viniul commented Oct 21, 2018

bshastry commented Oct 21, 2018

googlebot commented Oct 21, 2018

Dor1s commented Oct 21, 2018

Dor1s commented Oct 23, 2018

Add lzo #1862

Add lzo #1862

Conversation

bshastry commented Oct 8, 2018 • edited Loading

jonathanmetzman left a comment

Choose a reason for hiding this comment

jonathanmetzman Oct 8, 2018

Choose a reason for hiding this comment

bshastry Oct 8, 2018

Choose a reason for hiding this comment

jonathanmetzman Oct 8, 2018

Choose a reason for hiding this comment

bshastry Oct 10, 2018 • edited Loading

Choose a reason for hiding this comment

bshastry commented Oct 8, 2018

jonathanmetzman commented Oct 8, 2018

bshastry commented Oct 8, 2018

bshastry commented Oct 10, 2018 • edited Loading

googlebot commented Oct 10, 2018

bshastry commented Oct 10, 2018

viniul commented Oct 10, 2018

jonathanmetzman commented Oct 18, 2018

viniul commented Oct 18, 2018

viniul commented Oct 19, 2018

jonathanmetzman commented Oct 19, 2018

bshastry commented Oct 19, 2018

Dor1s commented Oct 19, 2018

viniul commented Oct 21, 2018

bshastry commented Oct 21, 2018

googlebot commented Oct 21, 2018

Dor1s commented Oct 21, 2018

Dor1s commented Oct 23, 2018

bshastry commented Oct 8, 2018 •

edited

Loading

bshastry Oct 10, 2018 •

edited

Loading

bshastry commented Oct 10, 2018 •

edited

Loading