guzba / zippy

Pure Nim implementation of deflate, zlib, gzip and zip.
MIT License
246 stars 29 forks source link

Instruction abort on Alpine x86_64 3.17.3 #59

Closed HookedBehemoth closed 1 year ago

HookedBehemoth commented 1 year ago

Tried updating my nitter instance, which updated zippy since then. Getting instruction aborts in inflateBlock. Rolling back to 0.9.11 works fine.

localhost:~/zippy# nimble --version
nimble v compiled at 2022-08-05 03:01:31
git hash: 43804b9c6ecec455a6851242a45a19b31b9b0776

localhost:~/zippy# nim --version
Nim Compiler Version 1.6.8 [Linux: amd64]
Compiled at 2022-11-15
Copyright (c) 2006-2021 by Andreas Rumpf

active boot switches: -d:release

localhost:~/zippy# gcc --version
gcc (Alpine 12.2.1_git20220924-r4) 12.2.1 20220924

Steps to reproduce:

localhost:~/zippy# nimble test -d:Release
  Verifying dependencies for zippy@0.10.7
  Compiling /root/zippy/tests/test (from package zippy) using c backend
randtest1.gz compressed: 4119 gold: 4096
randtest2.gz compressed: 8215 gold: 8192
randtest3.gz compressed: 12311 gold: 12288
rfctest1.gz compressed: 9370 gold: 34127
SIGILL: Illegal operation.
Error: execution of an external program failed: '/root/zippy/tests/test '
       Tip: 1 messages have been suppressed, use --verbose to show them.
     Error: Execution failed with exit code 1
        ... Command: /usr/bin/nim c --noNimblePath -d:NimblePkgVersion=0.10.7 --hints:off -d:Release -r --path:. /root/zippy/tests/test

Works fine in debug, which suggest that there is some UB at play. GDB points to this bit

    literalsHuffman = initHuffman(fixedLitLenCodeLengths)
    distancesHuffman = initHuffman(fixedDistanceCodeLengths)
  else:
    let
>      hlit = b.readBits(5).int + 257
      hdist = b.readBits(5).int + 1
      hclen = b.readBits(4).int + 4

cpuinfo.txt test.zip

guzba commented 1 year ago

Thanks for reporting the issue.

I set up a Debian VM and updated to GCC 12.1 and was not able to reproduce this issue unfortunately.

I started to set up an Alpine VM but realized I was out of my depth. It seemed to need manual networking set-up and apk just said "no such package" for everything. I enjoy programming but do not enjoy Linux sysadmin at all so I think you'll want to stick to the version that works until and unless I can reproduce this in a sane way.

HookedBehemoth commented 1 year ago

If it helps, I've setup a user with your public keys on my alpine server. https://api.github.com/users/guzba/keys ssh zippy@82.165.248.46

guzba commented 1 year ago

Ah interesting, I did not expect that at all. This may make reproducing and fixing very easy. I hope to give this a try tomorrow. Thanks.

guzba commented 1 year ago

Thanks again for setting that up. Unfortunately it is asking me for a password even if I specify a key that is one of those public keys such as ssh -i ~/.ssh/id_ed25519 zippy@82.165.248.46. Am I perhaps missing / not doing something quite right?

HookedBehemoth commented 1 year ago

Sorry, my fault. Can you try again?

guzba commented 1 year ago

Thanks again. I was able to identify the single commit that caused the crash (and did not crash when reverted). I'll look into the why shortly, but for now getting it reverted is a good start.

guzba commented 1 year ago

Reverting here https://github.com/guzba/zippy/pull/60

guzba commented 1 year ago

The revert is tagged in 0.10.8. It is harmless to others and should no longer fail for you. Could you verify if the tests now pass with this new version?

guzba commented 1 year ago

I think the issue here was related to alignment. I can see how that would work out as a cause. We'll never really know though. Closing this. Please re-open if the issue is still happening.

HookedBehemoth commented 1 year ago

Finally got to testing this. It's fixed now. Thanks a lot.