Skip to content
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.

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

Bug on 32 bit systems? #56

Open
jgm opened this issue Dec 6, 2023 · 19 comments
Open

Bug on 32 bit systems? #56

jgm opened this issue Dec 6, 2023 · 19 comments

Comments

@jgm
Copy link

jgm commented Dec 6, 2023

Debian packaging ran into a problem with pandoc on i386, described at jgm/pandoc#9233. I think I've traced it to base64. Here is a Dockerfile that reproduces the segfault:

FROM i386/debian:sid
RUN echo 'deb-src http://deb.debian.org/debian/ unstable main' >> /etc/apt/sources.list
RUN apt update
RUN apt install -y cabal-install libghc-base64-dev libghc-bytestring-dev
RUN printf 'import "base64" Data.ByteString.Base64 (encodeBase64)\n\
import qualified Data.ByteString as B\n\
main = B.readFile "/usr/bin/whoami" >>= print . encodeBase64\n'\
> test.hs
RUN runghc -XPackageImports test.hs
@iliastsi
Copy link

iliastsi commented Dec 6, 2023

I reproduced this with ghc-9.0 as well (i.e., use debian:stable instead of debian:sid), in case this helps. In addition, run the base64 testsuite on 32-bits fails as well (with segmentation fault).

@jgm
Copy link
Author

jgm commented Dec 6, 2023

Nothing special here about /usr/bin/whoami - I just chose a binary I was sure would be available. If you use B.take n to trim down the size of the bytestring that is converted, you'll eventually stop getting the segfault. What I found is that:

  • The behavior is not entirely deterministic, as I would get different results sometimes on different runs
  • Between the segfault and an errorless conversion (I didn't check it for correctness), I would sometimes get errors about improper UTF-8 encoding from Data.Text.Encoding. This suggests to me that the bytestring version of the base64 string is not all ASCII, as it should be. But I didn't investigate that further.

@emilypi
Copy link
Owner

emilypi commented Dec 7, 2023

Hi John, I deleted all of my 32 bit code because I don't want to support it anymore. Do you have a dire need for it? Also, what version of base64?

@emilypi
Copy link
Owner

emilypi commented Dec 7, 2023

Looking into it, I'd wager the memory boundaries for peek got a little stricter in GHC versions in the 9.x series which makes this line:

A little less sound. I can have a fix out rather quickly, but I'd need test data. Can you provide the offending image so i can add it as a regression? I can just add that to the test suite and support i386 in CI without adding my word32-specific code back into the repo

@jgm
Copy link
Author

jgm commented Dec 7, 2023

The Docker container above gives you everything you need to reproduce the problem. Any medium sized binary will work (I just use the whoami executable in the above test, but you could probably use any image).

As noted above, I've been able to reproduce problems (but not deterministically) with even fairly short bytestring literals.

There is no urgency on my end, because I already switched back to using base64-bytestring in pandoc.

@emilypi
Copy link
Owner

emilypi commented Dec 7, 2023

If performance isn't your bottleneck, that's certainly a solution. It uses the worst-case-in-every-case approach. That said, on the branch for #57 I can't get it to trigger, even with multiple 32-bit CI attempts, so maybe the fix diagnosis was spot on? I'll need a regression tho, so if you end up with something that triggers it more deterministically than not, I'm all ears. I could also just attempt to encode/decode something against whoami and the like to regress, but i have no reasonable means of testing on 32 bit systems.

@jgm
Copy link
Author

jgm commented Dec 7, 2023

With whoami, it never failed to segfault for me. I only got indeterministic results for very short bytestrings (e.g. the first 20 bytes).

Are there benchmarks of base64 vs bytestring-base64? I may go back if the problem is fixed.

@emilypi
Copy link
Owner

emilypi commented Dec 7, 2023

Small bytes

Okay - thanks i'll focus on that and see what i can dredge up. My guess is that when peeking a 64 byte word off, we cross some memory barrier somewhere that gets tripped in 32 bit systems when there are fewer than 2 words left in the array. The fix should target that case, since prior to the fix, we were looking only if there were at least six bytes left, not necessarily a full word, and GHC was always lax enough to say "if we attempt to read a full word and it's partial, fill with \NUL." It's actually probably saner to have this fix in regardless.

Are there benchmarks of base64 vs bytestring-base64? I may go back if the problem is fixed.

Benchmarks exist here, and do compare encodeBase64 and decodeBase64 against the base64-bytestring library. I maintain both libraries, so I regress against my worst case in this package alot of the time :)

Here's a sample of all recent output: https://github.com/emilypi/base64/blob/72cfd854ee3ba394e6dd7cfa0473d0fe542bf8ad/output.html.

The long short of it is that base64 gets away with an encode that's roughly 80%-100% faster, and decode is roughly 25%-40% faster for the typed loops, with negligible difference in the untyped loops. So, if you plan on encoding, and in particular encoding large numbers of bytes, I'd use this library. If you're primarily decoding, it's your pick. if you're only encoding small bytes, however, i'd honestly probably go with just the base64-bytestring package just because that one has fewer dependencies and the difference you'll see is minimal at best.

image

@jgm
Copy link
Author

jgm commented Dec 7, 2023

OK, thanks, that's helpful.

By the way, the initial image I tested with is test/lalune.jpg in the pandoc source distribution. Feel free to use that. But I think just about any image would work.

@emilypi
Copy link
Owner

emilypi commented Dec 8, 2023

After discussion with @kozross, I'm pretty sure the issue is as described. Merging and releasing

@jgm
Copy link
Author

jgm commented Dec 8, 2023

I don't see a new release yet?

@emilypi
Copy link
Owner

emilypi commented Jan 11, 2024

Got caught up in the holidays and forgot. It's up now: https://hackage.haskell.org/package/base64-1.0

@emilypi emilypi closed this as completed Jan 11, 2024
@emilypi emilypi reopened this Jan 11, 2024
@emilypi
Copy link
Owner

emilypi commented Jan 11, 2024

Actually, keeping this open just to confirm.

@dpwiz
Copy link

dpwiz commented Apr 3, 2024

Don't know if that's related, but we've been hit by SIGBUS/unaligned access on 32bit armv7a device 😟
(base64-1.0 on GHC-8.10.7)

@emilypi
Copy link
Owner

emilypi commented Apr 3, 2024

@dpwiz could you give more about the details? My suspicion would immediately be the encoding loop in that situation, but that's probably because we use 64-bit intrinsics in that particular hotloop. This library dropped support for 32 bit arches a few years ago.

@dpwiz
Copy link

dpwiz commented Apr 4, 2024

This library dropped support for 32 bit arches a few years ago.

Oops... Nvm then (:

@emilypi
Copy link
Owner

emilypi commented Apr 8, 2024

I'd still like to figure out how to solve it 😄

@dpwiz
Copy link

dpwiz commented Apr 15, 2024

Well, that was some android mobile running 32bit/v7 chip. Apparently those can't tolerate unaligned reads, so the app got killed.
I don't know much of the details, only that SIGBUS error code and the env description. Should be reproducible in a faithful QEMU I think.

@emilypi
Copy link
Owner

emilypi commented Apr 15, 2024

I'll see if I can work it out - thanks for the lead @dpwiz :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants