Current master fails with image #2565

[mohamedchebbii]

Reading the same image here : #2565 (comment)

./bin/exiv2 -pa   ~/img.jpg
Exif.Image.ImageWidth                        Long        1  4032
Exif.Image.ImageLength                       Long        1  3024
Exif.Image.Make                              Ascii       8  samsung
Exif.Image.Model                             Ascii       9  SM-G930V
Exif.Image.Orientation                       Short       1  right, top
Exif.Image.XResolution                       Rational    1  72
Exif.Image.YResolution                       Rational    1  72
Exif.Image.ResolutionUnit                    Short       1  inch
Exif.Image.Software                          Ascii      14  G930VVRS4BRA1
Exif.Image.DateTime                          Ascii      20  2018:04:10 18:28:03
Exif.Image.YCbCrPositioning                  Short       1  Centered
Exif.Image.ExifTag                           Long        1  238
Exif.Photo.ExposureTime                      Rational    1  1/3224 s
Exif.Photo.FNumber                           Rational    1  F1.7
Exif.Photo.ExposureProgram                   Short       1  Auto
Exif.Photo.ISOSpeedRatings                   Short       1  40
Exif.Photo.ExifVersion                       Undefined   4  2.20
Exif.Photo.DateTimeOriginal                  Ascii      20  2018:04:10 18:28:03
Exif.Photo.DateTimeDigitized                 Ascii      20  2018:04:10 18:28:03
Exif.Photo.ComponentsConfiguration           Undefined   4  YCbCr
Exif.Photo.ShutterSpeedValue                 SRational   1  1/3223 s
Exif.Photo.ApertureValue                     Rational    1  F1.7
Exif.Photo.BrightnessValue                   SRational   1  9.46
Exif.Photo.ExposureBiasValue                 SRational   1  0 EV
Exif.Photo.MaxApertureValue                  Rational    1  F1.7
Exif.Photo.MeteringMode                      Short       1  Center weighted average
Exif.Photo.LightSource                       Short       1  Unknown
Exif.Photo.Flash                             Short       1  No flash
Exif.Photo.FocalLength                       Rational    1  4.2 mm
Exif.Photo.MakerNote                         Undefined  98  7 0 1 0 7 0 4 0 0 0 48 49 48 48 2 0 4 0 1 0 0 0 0 32 1 0 12 0 4 0 1 0 0 0 0 0 0 0 16 0 5 0 1 0 0 0 90 0 0 0 64 0 4 0 1 0 0 0 0 0 0 0 80 0 4 0 1 0 0 0 1 0 0 0 0 1 3 0 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0
Exif.Photo.UserComment                       Undefined 4980  charset=Ascii 

Exif.Photo.FlashpixVersion                   Undefined   4  1.00
Exif.Photo.ColorSpace                        Short       1  sRGB
Exif.Photo.PixelXDimension                   Long        1  4032
Exif.Photo.PixelYDimension                   Long        1  3024
Exif.Photo.InteroperabilityTag               Long        1  5810
Exif.Iop.InteroperabilityIndex               Ascii       4  R98
Exif.Iop.InteroperabilityVersion             Undefined   4  1.00
Exif.Photo.SensingMethod                     Short       1  One-chip color area
Exif.Photo.SceneType                         Undefined   1  Directly photographed
Exif.Photo.ExposureMode                      Short       1  Auto
Exif.Photo.WhiteBalance                      Short       1  Auto
Exif.Photo.FocalLengthIn35mmFilm             Short       1  26.0 mm
Exif.Photo.SceneCaptureType                  Short       1  Standard
Exif.Photo.ImageUniqueID                     Ascii      11  C12QLJK01SM
Exif.Image.GPSTag                            Long        1  5840
Exif.GPSInfo.GPSVersionID                    Byte        4  2.2.0.0
Exif.GPSInfo.GPSLatitudeRef                  Ascii       2  North
Exif.GPSInfo.GPSLatitude                     Rational    3  37 deg 47' 9.80"
Exif.GPSInfo.GPSLongitudeRef                 Ascii       2  West
Exif.GPSInfo.GPSLongitude                    Rational    3  122 deg 23' 27.51"
Exif.GPSInfo.GPSAltitudeRef                  Byte        1  Above sea level
Exif.GPSInfo.GPSAltitude                     Rational    1  0.0 m
Exif.GPSInfo.GPSTimeStamp                    Rational    3  01:27:52
Exif.GPSInfo.GPSDateStamp                    Ascii      11  2018:04:11
Exif.Thumbnail.ImageWidth                    Long        1  504
Exif.Thumbnail.ImageLength                   Long        1  376
Exif.Thumbnail.Compression                   Short       1  JPEG (old-style)
Exif.Thumbnail.Orientation                   Short       1  right, top
Exif.Thumbnail.XResolution                   Rational    1  72
Exif.Thumbnail.YResolution                   Rational    1  72
Exif.Thumbnail.ResolutionUnit                Short       1  inch
Exif.Thumbnail.JPEGInterchangeFormat         Long        1  6176
Exif.Thumbnail.JPEGInterchangeFormatLength   Long        1  13544

[ghost]

👇 Click on the image for a new way to code review

Legend

[benmccann]

I don't know much C++, so forgive me if I'm very wrong, but I don't understand the reasoning behind adding a +1 in these locations. There are lots of places buffers are created (egrep -r "buf\(" src/), so why only add it in these places instead of all others? I think @neheb's second suggestion is trying to add it everywhere, so may make more sense in that regard. But even still, if we add it everywhere, why should it be size + 1? Do we need an extra byte for a null termination character or something?

[mohamedchebbii]

Hello @ben,
Please ignore my previous comment,
No you are right, in c++ we don't need to add +1 to allocate table of byte.
We just need to keep in memory that when declaring a table of n size, index are for 0 to n-1.
The issue here that when reading a segment block ( segement is structured as follow: size (2byte) + conten (size in the first two bytes)).
The segement content size is in the first two bytes.
In this specific image, we have only the first part (the size=2)
so when trying to read to rest of the segment in line 358:jpgimage.cpp

io_->readOrThrow(buf.data(2), size - 2, ErrorCode::kerFailedToReadImageData);

buf.data(2) : we try to read size-2=0 bytes from the third byte : segmentation fault becaue the size of buffer is 2.

[codecov]

Codecov Report

Merging #2586 (f790c20) into main (fbc0b21) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2586   +/-   ##
=======================================
  Coverage   63.92%   63.92%           
=======================================
  Files         103      103           
  Lines       22374    22311   -63     
  Branches    10823    10795   -28     
=======================================
- Hits        14302    14262   -40     
+ Misses       5851     5828   -23     
  Partials     2221     2221           

Impacted Files	Coverage Δ
src/jpgimage.cpp	68.41% <100.00%> (ø)

... and 26 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[benmccann]

Thanks @mohamedchebbii! Would you be able to add a test with the image from #2565 to prove it works and prevent regressions?

[benmccann]

thank you!!