Several fixes to remove all build warnings and improve stability #320

FrostyHitoshura · 2018-08-12T20:27:00Z

All these changes were done under Fedora 28 x86_64 (gcc 8.1.1). There are good chances that these changes will be beneficial to other Linux distributions as well.

The changes to "src/video/video_sdl2.c" are the most critical as without these, I don't consider the game to be playable on my setup.

…m libc

…e set this as an inline function since it's not used in all configurations.

* Handling of the mouse coordinates is not straightforward due to the use of the logical renderer feature. Without the change, the game would randomly crash due to memory corruption when the cursor would go outside the logical area. * When doing transitions between fullscreen and windowed modes, the display would contain graphical artifacts until a full repaint was done. We now force a render when we get an SDL expose event to correct this.

miniupnp

there a quite few issue with your commits.
I also need to test the sdl2 changes on more configurations

miniupnp · 2018-08-12T20:33:29Z

src/audio/driver.c

@@ -329,7 +329,7 @@ void Driver_Sound_LoadFile(const char *musicName)
 char *Drivers_GenerateFilename(const char *name, Driver *driver)
 {
 	char basefilename[14];
-	static char filename[14];
+	static char filename[18];


why ?
filename is 8.3
And no stack overflow is possible with snprintf, right ?

This buffer size was changed for the following statement:

snprintf(filename, sizeof(filename), "%s.%s", basefilename, driver->extension);

Without the change, gcc spew the following:

[...]/OpenDUNE/src/audio/driver.c: In function ‘Drivers_GenerateFilename’: [...]l/OpenDUNE/src/audio/driver.c:341:43: warning: ‘%s’ directive output may be truncated writing up to 3 bytes into a region of size between 0 and 13 [-Wformat-truncation=] snprintf(filename, sizeof(filename), "%s.%s", basefilename, driver->extension); ^~

The next call to snprintf in this function also generates a similar warning.

In this case, gcc figures out that we want to concatenate 2 strings, the first can be at most 13 bytes long (sizeof(basefilename) - 1 => 13), one dot (1 byte) and finally (sizeof(driver->extension) - 1 => 3). Summing this up gives us 17 and add 1 for the terminal NUL, that's why I extended the buffer to 18 bytes, which fixes the warning.

While we are already using snprintf, while this fixes potential buffer overflows which is a good thing, it doesn't fully fix the problem as far as the user is concerned because the application isn't going to work correctly.

With the information given to the compiler, it's not able to deduce we want to build a FAT16 (8.3) file name.

so maybe the proper fix is to reduce basefilename to 9 chars ?

we could remove the use of basefilename and snprintf altogether and rewrite the whole function ;)

Or use:

snprintf(filename, sizeof(filename), "%.8s.%.3s", basefilename, driver->extension);

which maximizes each string input length

miniupnp · 2018-08-12T20:34:40Z

src/gui/gui.c

 	char buf[100];
 	uint16 i;

-	sprintf(category, "GROUP%d", campaignID);
+	sprintf(category, "GROUP%hu", campaignID);


better change to snprintf()

instead of increasing category buffer size I mean.

In this case, gcc sees that a %d (int) on x86_64 is 32 bits (signed) and %d can produce at most 11 bytes == strlen("-2147483648"). A half unsigned int (%hu), which will output at most 5 bytes == strlen("65535") fits the output buffer.

Since we have a reoccurring problem with s*printf and length of output buffer and all(?) cases are similar, we try to build a 8.3 file name based on a 5 bytes base, 3 bytes number and finally the extension, maybe it would be worthwhile to build our own function to build these and avoid the complication of s*printf altogether.

I vote for snprintf(), easiest fix and will work on any sizes of int in the future as well

miniupnp · 2018-08-12T20:35:06Z

src/gui/widget_click.c

-	static char filename[13];
-	sprintf(filename, "_save%03d.dat", number);
+	static char filename[15];
+	sprintf(filename, "_save%03hx.dat", number);


it should be %03hu !!!

and filename is 8.3 for sure ! use snprintf() instead of increasing filename buffer

Correct, I messed this one up, it should be "%03hu". Note that since even if we specify that the number should be prefixed by 3 zeroes, if you attempt to print 65535, it will output that, not 535 so that's why I had to increase the output buffer again.

but number should never be > 999

either use snprint and/or clamp number to not be bigger than 999 ?

miniupnp · 2018-08-12T20:37:26Z

src/os/strings.h

@@ -84,13 +84,13 @@

 	#if !defined(__MINGW32__) && defined(__GNUC__) && !defined(snprintf)
 		/* (v)snprintf is in fact C99, but we like to use it over (v)sprintf for the obvious reasons */
-		#if !defined(__APPLE__) && !defined(TOS) && !defined(__FreeBSD__) && !defined(__OpenBSD__) && !defined(__DJGPP__)
+		#if !defined(__APPLE__) && !defined(TOS) && !defined(__FreeBSD__) && !defined(__OpenBSD__) && !defined(__DJGPP__) && !defined(_XOPEN_SOURCE) && (defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE < 500))


there is a logical impossibility :
!defined(_XOPEN_SOURCE) && (defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE < 500))
Do you mean
(!defined(_XOPEN_SOURCE) || (defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE < 500))) ?

The plain english version of what I attempted to code here:

If you do not have _XOPEN_SOURCE, use the definitions below.

If you do have _XOPEN_SOURCE and it's below 500 (value which I determined by reading strdup(3) and printf(3) on my system), use the definitions below.

I think that your implementation fits this description, mine doesn't after carefully reviewing this in my head. I also found why it was working on my machine, the related change I've done to "config.lib" is incomplete and running:

make VERBOSE=1

did reveal that I didn't have "-D_XOPEN_SOURCE=500" like I was expecting. I will have to fix this mess...

miniupnp · 2018-08-12T20:37:52Z

src/os/strings.h

 			extern int snprintf (char *__restrict __s, size_t __maxlen, __const char *__restrict __format, ...) __THROW __attribute__ ((__format__ (__printf__, 3, 4)));
 			extern int vsnprintf (char *__restrict __s, size_t __maxlen, __const char *__restrict __format, va_list __arg) __THROW __attribute__ ((__format__ (__printf__, 3, 0)));
 		#endif /* __APPLE__ */
 	#endif /* __GCC__ */

-	#if !defined(__MINGW32__) && defined(__GNUC__) && !defined(strdup) && !defined(__APPLE__) && !defined(TOS) && !defined(__FreeBSD__) && !defined(__DJGPP__)
+	#if !defined(__MINGW32__) && defined(__GNUC__) && !defined(strdup) && !defined(__APPLE__) && !defined(TOS) && !defined(__FreeBSD__) && !defined(__DJGPP__) && !defined(_XOPEN_SOURCE) && (defined(_XOPEN_SOURCE) && (_XOPEN_SOURCE < 500))


same a above

miniupnp · 2018-08-12T20:39:24Z

src/video/scale2x.h

@@ -65,7 +65,7 @@ typedef uint32_t scale2x_uint32;
 /**
 * Align a pointer.
 */
-static void* scale2x_align_ptr(const void* ptr)
+static inline void* scale2x_align_ptr(const void* ptr)


that commit make the build fail :
https://travis-ci.org/OpenDUNE/OpenDUNE/builds/415198858

I can't reproduce this and I was very puzzled what was going on. Adding "inline" to functions in header file isn't a first for me but that it would break a build, yes.

I suspect that since there's no mention of which version of the C standard in CFLAGS, each compiler will select a different one, that's why we see this discrepancy between environments.

I see that at various place in the code, we have similar constructs:

#ifdef _MSC_VER #define inline __inline #define restrict __restrict #else /* _MSC_VER */ #define inline __inline__ #define restrict __restrict__ #endif /* _MSC_VER */

I could try to add that at the beginning of "scale2x.h" which might work... or not because if previously in a compilation unit we try to "#define include" again, it will break.

We also have another attempt to do something similar in "config.lib":

CFLAGS="$CFLAGS -Dinline=__inline__"

Because of all these reasons, I think there's 2 ways to solve this problem once and for all:

Force a C standard in CFLAGS which have a definition for "inline".

Remove all attempt in the code base to #define include (and friends) and create a new header file, let's call it "compiler.h" and add all these defines here. All #define sites we removed now should #include "compiler.h" instead. This new header will be protected by "include guards" so if a compilation unit #include "compiler.h" more than once, it will not break as we will not "#define inline" more than once.

The second approach looks better to me long term as if other compiler specific idiosyncrasies do popup, they can be added to that file.

https://github.com/OpenDUNE/OpenDUNE/wiki/Coding-Style
inline is not C89

FrostyHitoshura · 2018-08-17T02:08:09Z

I will wait for your report on the SDL2 code and your replies to the various propositions I have done before I submit an updated pull request.

miniupnp · 2018-08-19T05:13:22Z

SDL2 code looks good, I will already merge it

FrostyHitoshura · 2018-08-19T19:23:03Z

Perfect! There are other change sets that you didn't comment on and maybe could be merged in your master as-is:

I will rework the other changes and come back with another pull request.

mywave82

config.lib script should probably look like this (keep the original block for adding -DUNIX) and add a new block that also allows $os to be UNIX:

    # Most targets act like UNIX, just with some additions
    if [ "$os" = "BEOS" ] || [ "$os" = "HAIKU" ] || [ "$os" = "OSX" ] || [ "$os" = "MORPHOS" ] || [ "$os" = "FREEBSD" ] || [ "$os" = "OPENBSD" ] || [ "$os" = "NETBSD" ] || [ "$os" = "HPUX" ] || [ "$os" = "SUNOS" ] || [ "$os" = "OS2" ]; then
            CFLAGS="$CFLAGS -DUNIX"
    fi

    if [ "$os" = "BEOS" ] || [ "$os" = "HAIKU" ] || [ "$os" = "OSX" ] || [ "$os" = "MORPHOS" ] || [ "$os" = "FREEBSD" ] || [ "$os" = "OPENBSD" ] || [ "$os" = "NETBSD" ] || [ "$os" = "HPUX" ] || [ "$os" = "SUNOS" ] || [ "$os" = "OS2" ] || [ "$os" = "UNIX" ]; then
            # We want "strdup" and "*snprintf" from libc
            CFLAGS="$CFLAGS -D_XOPEN_SOURCE=500"
    fi

FrostyHitoshura added 6 commits August 12, 2018 15:29

-Fix: Typo

589d8a5

-Build: On Linux/Unix, use the "strdup" and "*snprintf" functions fro…

b536321

…m libc

-Fix: Several potential stack overflow reported by gcc

cf4706e

-Build: Fixed build warning "variable set but not used"

ae129d7

-Build: Fix build warning "scale2x_align_ptr defined but not used", w…

c5ee2fe

…e set this as an inline function since it's not used in all configurations.

miniupnp requested changes Aug 12, 2018

View reviewed changes

mywave82 suggested changes Sep 24, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Several fixes to remove all build warnings and improve stability #320

Several fixes to remove all build warnings and improve stability #320

FrostyHitoshura commented Aug 12, 2018

miniupnp left a comment

miniupnp Aug 12, 2018

FrostyHitoshura Aug 17, 2018

miniupnp Aug 19, 2018

miniupnp Aug 19, 2018

mywave82 Sep 24, 2018

miniupnp Aug 12, 2018

miniupnp Aug 12, 2018

FrostyHitoshura Aug 17, 2018 •

edited

mywave82 Sep 24, 2018

miniupnp Aug 12, 2018

miniupnp Aug 12, 2018

FrostyHitoshura Aug 17, 2018

miniupnp Aug 19, 2018

mywave82 Sep 24, 2018

miniupnp Aug 12, 2018

FrostyHitoshura Aug 17, 2018

miniupnp Aug 12, 2018

miniupnp Aug 12, 2018

FrostyHitoshura Aug 17, 2018

miniupnp Aug 19, 2018

FrostyHitoshura commented Aug 17, 2018

miniupnp commented Aug 19, 2018

FrostyHitoshura commented Aug 19, 2018

mywave82 left a comment •

edited

Several fixes to remove all build warnings and improve stability #320

Are you sure you want to change the base?

Several fixes to remove all build warnings and improve stability #320

Conversation

FrostyHitoshura commented Aug 12, 2018

miniupnp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrostyHitoshura Aug 17, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrostyHitoshura commented Aug 17, 2018

miniupnp commented Aug 19, 2018

FrostyHitoshura commented Aug 19, 2018

mywave82 left a comment • edited

Choose a reason for hiding this comment

FrostyHitoshura Aug 17, 2018 •

edited

mywave82 left a comment •

edited