Skip to content

Fix undefined-shift in scanline_read_old #1727

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 21, 2020
Merged

Fix undefined-shift in scanline_read_old #1727

merged 2 commits into from
Jul 21, 2020

Conversation

kleisauke
Copy link
Member

See for details: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=24230.

As an aside,
I somehow couldn't reproduce this within the Travis build, see:
https://travis-ci.org/github/kleisauke/libvips/jobs/709682348

Perhaps there's a -fsanitize= check missing within our flags?

libvips/.travis.yml

Line 113 in 72ca38c

- CFLAGS="-fsanitize=address,undefined -fno-omit-frame-pointer -fopenmp -DFUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION"

For reference, here are the undefined sanitizer flags set by OSS-Fuzz:
https://github.com/google/oss-fuzz/blob/b4bf7839051cc7bcc9f4d1989eaab82865bae367/infra/base-images/base-builder/Dockerfile#L93

lovell
lovell approved these changes Jul 19, 2020
View reviewed changes
Copy link
Member

@lovell lovell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Would -fsanitize=shift catch this?

jcupitt
jcupitt reviewed Jul 19, 2020
View reviewed changes
@kleisauke
Explicitly cast to unsigned int before left-shifting
aac1748 
To avoid potential signed integer overflow (undefined behavior), as
implicit integer promotion means the operand becomes a (signed) int.
@kleisauke
Copy link
Member Author

It seems that UBSan didn't exits with a non-zero code on error. Commit kleisauke@5f0abac on the oss-fuzz-24230-travis branch resolves this. However it revealed new UBSan errors on the existing corpus, see:
https://travis-ci.org/github/kleisauke/libvips/jobs/709752013

flatten.c:190:4: runtime error: 1.64485e+07 is outside the range of representable values of type 'unsigned short'
scRGB2sRGB.c:114:11: runtime error: 8224 is outside the range of representable values of type 'unsigned char'
mosaic_fuzzer.cc:47:56: runtime error: member access within misaligned address 0x60b0000004c3 for type 'struct mosaic_opt', which requires 2 byte alignment

I could resolve the last error with commit kleisauke@161e667, see:
https://travis-ci.org/github/kleisauke/libvips/jobs/709931677

I'm not sure what's causing these other errors, maybe a VIPS_CLIP is missing?

@jcupitt
Copy link
Member

jcupitt commented Jul 21, 2020

No, I'm not sure either. Let's merge this nice improvement for now and look at the other error in a new issue.

@jcupitt jcupitt merged commit 7c413f8 into libvips:master Jul 21, 2020
@kleisauke
Copy link
Member Author

Here's a work-in-progress branch to fix the remaining UBSan errors:
master...kleisauke:fix-ubsan-errors

Relevant Travis build:
https://travis-ci.org/github/kleisauke/libvips/jobs/710731533

A VIPS_CLIP was missing in vips_scRGB2sRGB_line_8. I also added this in vips_scRGB2BW_line_8, just to be on the safe side. I fixed the vips_flatten UBSan error with some saturated casts for the int types (copied from vips_cast).

It still needs some commit polishing and I'm not sure if there's a better way to fix that UBSan error in vips_flatten. What do you think?

@kleisauke kleisauke deleted the oss-fuzz-24230 branch July 22, 2020 13:22
@kleisauke kleisauke mentioned this pull request Dec 29, 2020
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcupitt jcupitt jcupitt left review comments

@lovell lovell lovell approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kleisauke @jcupitt @lovell