devshane / zork

The DUNGEON (Zork I) source
594 stars 128 forks source link

Fix reused integers from being optimized out #14

Closed jwflory closed 3 years ago

jwflory commented 3 years ago

Summary

Fix segfault when reused integers are optimized out by compiler

Background

This patch was contributed by @Jan200101 to fix a reported segfault in the Fedora package from compiler optimizations. We can carry a custom patch for the Fedora-specific package, but we generally try to work with an upstream so everyone may benefit from a change, not only Fedora.

This change was tested with a local scratch build on at least two Fedora machines. It is likely other distributions could face similar issues when upgrading their compilers.

Details

It is a simple change, and honestly I am not a C developer. If you really want an explanation, @Jan200101 could probably provide one if you ask.

If this change is accepted, it would be helpful to cut a new release with a git tag. This will trigger the first step in the Fedora package distribution process for shipping an update. 🙂

Outcome

Support optimizations from newer versions of compilers without segfaulting after opening the mailbox

Jan200101 commented 3 years ago

The problem was with how gcc optimized the program (gcc versions before 10.2.1 might not have this problem) compiling zork with -O2, as one does for Fedora packages, made the compiler try to optimize i__2 out (gdb output copied from the previously mentioned report)

#0  sparse_ (lbuf=0x7fffffffd54c, lbuf@entry=0x7fffffffd550, llnt=3, vbflag=vbflag@entry=1) at np1.c:128
        ret_val = -1
        i__1 = 3
        i__2 = <optimized out>
        i = 3
        j = 44617
        obj = <optimized out>
        prep = 0
        lbuf1 = 3258
        lbuf2 = 0

This being the code that triggers this sigsegv https://github.com/devshane/zork/blob/95b1fd1feead1d52ce079678070f37daaf66766d/np1.c#L126-L131

if we look at prplnt https://github.com/devshane/zork/blob/95b1fd1feead1d52ce079678070f37daaf66766d/np1.c#L58

we can tell something went horribly wrong in that gdb backtrace since j is far larger than prplnt the way to solve this is to tell the compiler not to optimize i__2 out, and in the case of my patch I made all system generated locals that are reused over the file volatile

jwflory commented 3 years ago

Thanks for clarifying the change @Jan200101.

@devshane Do you have a moment to review this patch? It would be great to add this here in the upstream project instead of maintaining a downstream patch only available in the Fedora package.

devshane commented 3 years ago

Thanks for the fix!

https://github.com/devshane/zork/releases/tag/v1.0.3