Avoid taking address of packed member

[Nurckye]

Compiling the codebase with -Waddress-of-packed-member produces multiple errors at build time:

 error: taking address of packed member 'bmpih' of class or structure 'BMP_HEADER' may result in an unaligned pointer value [-Werror,-Waddress-of-packed-member]
    compression = convertOnBigEnd32(bmpih->biCompression);

Taking the address of a packed member is dangerous since the reduced alignment of the pointee is lost. This can lead to memory alignment faults in some architectures if the pointer value is dereferenced.

[DanBloomberg]

I know that this approach is in general unsafe. However, for BMP it's a simple approach that works on all platforms tested and has been extensively fuzzed. Besides being simple, I'm following "if it isn't broken, don't 'fix' it".

If you'd like to redo the code to avoid the compiler warning, have at it :-)

[glaubitz]

I know that this approach is in general unsafe. However, for BMP it's a simple approach that works on all platforms tested and has been extensively fuzzed. Besides being simple, I'm following "if it isn't broken, don't 'fix' it".

It actually crashes the testsuite on sparc64:

FAIL: ioformats_reg
===================

////////////////////////////////////////////////
////////////////   ioformats_reg   ///////////////
////////////////////////////////////////////////
leptonica-1.84.2 : libgif 5.2.2 : libjpeg 6b (libjpeg-turbo 2.1.5) : libpng 1.6.44 : libtiff 4.5.1 : zlib 1.3.1 : libwebp 1.4.0 : libopenjp2 2.5.0
Test bmp 1 bpp file:
Bus error (core dumped)
FAIL ioformats_reg (exit status: 138)

with the following backtrace:

#0  0xfff800010064f9b0 in pixReadMemBmp (cdata=0x10000231160 "BMV\r", size=3414) at bmpio.c:181
181         ihbytes = convertOnBigEnd32(*(l_uint32 *)(bmpih));
(gdb) bt
#0  0xfff800010064f9b0 in pixReadMemBmp (cdata=0x10000231160 "BMV\r", size=3414) at bmpio.c:181
#1  0xfff80001006504dc in pixReadStreamBmp (fp=0x1000022c1d0) at bmpio.c:102
#2  0xfff80001007aa700 in pixReadStream (fp=0x1000022c1d0, hint=0) at readfile.c:329
#3  pixReadStream (fp=0x1000022c1d0, hint=0) at readfile.c:313
#4  0xfff80001007aa8b8 in pixRead (filename=0x10000231140 "./weasel2.4c.bmp") at readfile.c:200
#5  pixRead (filename=0x10000231140 "./weasel2.4c.bmp") at readfile.c:189
#6  0xfff80001007f538c in writeMultipageTiffSA (sa=0x10000204ac0, fileout=0x10000002aa8 "/tmp/lept/tiff/weasel_orig.tif") at tiffio.c:1579
#7  0xfff80001007f551c in writeMultipageTiff (dirin=0x10000002860 ".", substr=0x10000002ac8 "weasel2", fileout=0x10000002aa8 "/tmp/lept/tiff/weasel_orig.tif")
    at tiffio.c:1536
#8  0x0000010000001f1c in main (argc=<optimized out>, argv=<optimized out>) at mtiff_reg.c:270

Full build log at: https://buildd.debian.org/status/fetch.php?pkg=leptonlib&arch=sparc64&ver=1.84.1-4&stamp=1728101108&raw=0

[DanBloomberg]

All 3 test failures on sparc64 (ioformats_reg, mtiff_reg and pngio_reg) are due to this alignment problem reading bmp into memory.

[DanBloomberg]

Thank you for filing the detailed bug report. I believe this is now fixed. Please verify on the sparc64 architecture.

[stweil]

I'm afraid it is now wrong on big endian machines, but can make a test and send a fix.

[DanBloomberg]

Thanks, Stefan. It works on little-endians, which is why I added the call to convertOnBigEnd32((), which is used on all the 32-bit data.

[glaubitz]

I can confirm that the alignment issues are gone but the testsuite is now failing for different reasons (sparc64 is big-endian as well):

////////////////////////////////////////////////
////////////////   rankhisto_reg   ///////////////
////////////////////////////////////////////////
leptonica-1.84.2 : libgif 5.2.2 : libjpeg 6b (libjpeg-turbo 2.1.5) : libpng 1.6.44 : libtiff 4.5.1 : zlib 1.3.1 : libwebp 1.4.0 : libopenjp2 2.5.0
Failure in rankhisto_reg, index 0: comparing /tmp/lept/regout/rankhisto.00.png with /tmp/lept/golden/rankhisto_golden.00.png
0: 926c3300
1: aa874f00
2: ac946400
3: b3a27000
4: b6ac7d00
5: 9ba89500
6: 8ca7a600
7: 90abab00
8: bfbaac00
9: e2d4c700
Warning: empty y range [1:1], adjusting to [0.99:1.01] 
Warning: empty y range [50:50], adjusting to [49.5:50.5]
Warning: empty y range [99:99], adjusting to [98.01:99.99]
Time:   2.941 sec
FAIL rankhisto_reg (exit status: 1)

[stweil]

Pull request #752 should fix it. I could not test rankhisto_reg because my test machine runs Solaris and does not provide the required gnuplot.

[glaubitz]

Pull request #752 should fix it. I could not test rankhisto_reg because my test machine runs Solaris and does not provide the required gnuplot.

Yes, that fixes it.

However, I'm left with one more failing test now:

////////////////////////////////////////////////
////////////////   crop_reg   ///////////////
////////////////////////////////////////////////
leptonica-1.84.2 : libgif 5.2.2 : libjpeg 6b (libjpeg-turbo 2.1.5) : libpng 1.6.44 : libtiff 4.5.1 : zlib 1.3.1 : libwebp 1.4.0 : libopenjp2 2.5.0
Page 5
Warning in pixReversalProfile: last > w - 1; clipping
Warning in pixAverageIntensityProfile: last > w - 1; clipping
txt1 = 35, pap1 = 35
txt2 = 381, pap2 = 664
Page 36
Warning in pixReversalProfile: last > w - 1; clipping
Warning in pixAverageIntensityProfile: last > w - 1; clipping
txt1 = 81, pap1 = 50
txt2 = 311, pap2 = 329
Writing profiles to /tmp/lept/crop/croptest.pdf

es
^
"/tmp/lept/gplot/pix1.4.cmd" line 4: invalid command

Failure in crop_reg, index 4: comparing /tmp/lept/regout/crop.04.png with /tmp/lept/golden/crop_golden.04.png
Failure in crop_reg, index 5: comparing /tmp/lept/regout/crop.05.png with /tmp/lept/golden/crop_golden.05.png
Time:   6.875 sec
FAIL crop_reg (exit status: 1)

Edit: This seems to be an unrelated failure and more like an issue with my environment.

[DanBloomberg]

Yes, I suspect it's a gnuplot issue in your environment. That command file only has 3 lines:

set title 'Reversals'
set terminal png; set output '/tmp/lept/gplot/pix1.4.png'
plot '/tmp/lept/gplot/pix1.4.data.1' title '' with lines

and for some reason the last two characters of the 3rd line have been put on line 4.

[stweil]

@glaubitz, pull request #753 adds changes which I needed for the test on Solaris. Maybe you can try whether they fix the test failure in your case, too.

[glaubitz]

@glaubitz, pull request #753 adds changes which I needed for the test on Solaris. Maybe you can try whether they fix the test failure in your case, too.

Yep, that fixes it:

============================================================================
Testsuite summary for leptonica 1.84.2
============================================================================
# TOTAL: 148
# PASS:  148
# SKIP:  0
# XFAIL: 0
# FAIL:  0
# XPASS: 0
# ERROR: 0
============================================================================

That's the master branch plus #752 and #753. Thanks for fixing it!

[stweil]

Thanks for the test!

[DanBloomberg]

The compiler warnings about taking the address of packed data is fixed with PR #754

[jeffbreidenbach]

Is the fix ready to be patched into Debian, or are things still in flux?

https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1084905

[stweil]

I suggest to use either the master branch (which contains additional improvements), or use at least src/bmpio.c from the master branch (which is sufficient to fix the reported bug).

[DanBloomberg]

If only taking files, you should also take src/bmp.h

[jeffbreidenbach]

I'm looking for guidance. I could wait for 1.85, wait for a 1.85 release candidate, or patch in src/bmp.[ch]. However, I don't want to use the master branch without clear version numbers involved; it would make bug reports too confusing. Please note that Debian had a memory alignment bug report from 1.84.1 involving the rust bindings that is getting figured out. It may be completely unrelated and just need regeneration of the bindings.

https://ci.debian.net/packages/r/rust-leptonica-sys/testing/i386/52950971/#S7

[DanBloomberg]

It's time I release 1.85. I'll try to do it in the next 2 days, and it will give you more options.

[jeffbreidenbach]

Based on previous experience, please consider calling it a release candidate. If no little gotchas are found then the release can be exactly the same as the release candidate.

[DanBloomberg]

I've always described the releases thus: This is a configure-ready release, derived from the master on ...

Do you want me to say instead: This is a configure-ready release candidate, derived ... ? If so, how does one re-classify it as a "release" later?

[stweil]

I don't expect little gotchas in the release because nearly all changes are fixes or better documentation as far as I see. Therefore I think that it's sufficient to make a normal release. If there really were problems with the new release, another bugfix release could follow.

[DanBloomberg]

OK. 1.85.0 is on github. I forgot that in my "version release" notes I'd indicated to check pre-release if Jeff was making a debian release as well. But since this is likely stable as is, it's a release.

[DanBloomberg]

Oops, didn't change CMakeLists version number. Hold off ...

[DanBloomberg]

1.85.0 should be OK.