Code cleanup

[CoSoCo]

I don't understand why the strings of the glob_prefs are initialised with the default values using the long switch ... case statement (from line 172) with the default values instead of using the compact jp_pref_init(...) function. In addition, it looks much more organised and clearer if strings are defined directly in the definition of the glob_prefs array.

It should also run faster as it saves from layawaying through several switch cases. It may also help to avoid issue #53 , because several svalue fields of glob_prefs stay NULL instead of being initialized to "" and svalue_size fields stay 0 instead 1. While developing my JPilotMediaPlugin (see from line 68) I often have seen such SIGCHLD crashes while reading or writing the prefs before I properly initialized (line 965) those fields.

And last, but not least, since strdup() allocates heap memory, isn't it also a bug that it is not freed anywhere, which is what the jp_free_prefs() function was intended for. This may also help to avoid issue #53 .

[eklausme]

I cannot accept this pull request.

Although your code clearly looks easier, it seems to be not correct, as the strings in question in prefType are used with realloc() in pref_lstrncpy_realloc(). Function realloc() only works with malloc()-ed storage, not with static/global storage.

Maybe pref_lstrncpy_realloc() can be changed to detect static/gloabl strings, which have length zero. But then, why add more code somewhere else, to get rid of some code in this place.

[CoSoCo]

Thanks for your deeper look.

Although your code clearly looks easier, it seems to be not correct, as the strings in question in prefType are used with realloc() in pref_lstrncpy_realloc(). Function realloc() only works with malloc()-ed storage, not with static/global storage.

I can not understand your concern, because AFAIK strdup(...) in jp_pref_init(...) already guarantees, that those strings are malloc()-ed.

Additionally I'm curious, why you removed the static modifier from prefType glob_prefs[NUM_PREFS] = {...};. There is only one prefs file, so allowing multiple instances of glob_prefs does not make sense.

Maybe pref_lstrncpy_realloc() can be changed to detect static/gloabl strings, which have length zero. But then, why add more code somewhere else, to get rid of some code in this place.

Anyway I think, it already does this detection by:

      if (*size == 0) {
         *dest=malloc(new_size);
      } else {
         *dest=realloc(*dest, new_size);
      }

But if all string fields in prefs[] are already initialized "properly" by jp_pref_init(...), this detection becomes superfluous.

Additionally I think, the code in pref_lstrncpy_realloc() could be stripped, which would make it better readable and saves from scolling:

{
   const char *Psrc = src ? src : "";
   int new_size = (strlen(Psrc)+1 > *size) ? strlen(Psrc)+1 : *size;
   if (new_size > max_size)  new_size = max_size;

   if (new_size > *size) {
      *dest = *size ? realloc(*dest, new_size) : malloc(new_size);
      if (!(*dest))
         return "";     // ***Problem***: This results in a static return value anyway !!
                        // So maybe better return `int -1` as error code, because the real value is never used.
/* Alternative, if all strings are already `malloc()`-ed by `jp_pref_init(...)`: */
//      if (!(*dest = realloc(*dest, new_size)))
//         return "";
      *size = new_size;
   }
   g_strlcpy(*dest, Psrc, new_size);

   return *dest;
}

And I suspect if the whole function may be superfluous, if jp_pref_init(...) internally already would make use of g_strlcpy(...).

[eklausme]

Here is an example realloctst.c that demonstrates what happens when you tamper with realloc() and non-heap storage.

#include <stdio.h>
#include <string.h>
#include <stdlib.h>

int main (int argc, char *argv[]) {
        char *s = "ABC";
        char *t = strdup("UVW");
        char *sr = realloc(s,1024);
        char *tr = realloc(t,1024);
        printf("s=%lx, t=%lx, sr=%lx, tr=%lx\n",
                (long)s,(long)t,(long)sr,(long)tr);
        return 0;
}

Compiling this code with gcc -Wall gives below warnings:

realloctest.c: In function ‘main’:
realloctest.c:10:9: warning: pointer ‘s’ used after ‘realloc’ [-Wuse-after-free]
   10 |         printf("s=%lx, t=%lx, sr=%lx, tr=%lx\n",
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   11 |                 (long)s,(long)t,(long)sr,(long)tr);
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
realloctest.c:8:20: note: call to ‘realloc’ here
    8 |         char *sr = realloc(s,1024);
      |                    ^~~~~~~~~~~~~~~
realloctest.c:10:9: warning: pointer ‘t’ used after ‘realloc’ [-Wuse-after-free]
   10 |         printf("s=%lx, t=%lx, sr=%lx, tr=%lx\n",
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   11 |                 (long)s,(long)t,(long)sr,(long)tr);
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
realloctest.c:9:20: note: call to ‘realloc’ here
    9 |         char *tr = realloc(t,1024);
      |                    ^~~~~~~~~~~~~~~
realloctest.c:8:20: warning: ‘realloc’ called on a pointer to an unallocated object ‘"ABC"’ [-Wfree-nonheap-object]
    8 |         char *sr = realloc(s,1024);
      |                    ^~~~~~~~~~~~~~~
realloctest.c:6:15: note: assigned here
    6 |         char *s = "ABC";
      |               ^

Syntax is correct. Code looks "clean".

The problematic part here is: ‘realloc’ called on a pointer to an unallocated object ‘.

If you run the program:

./realloctest
zsh: segmentation fault (core dumped)  ./realloctest

That's the best what can happen ;-)

[CoSoCo]

... warning: pointer ‘s’ used after ‘realloc’ The problematic part here is: ‘realloc’ called on a pointer to an unallocated object ‘.

Correct. The problem happens, because your sample code copies the new pointer from realloc() to another variable sr and then after reuses the old unchanged value of s.

But in pref_lstrncpy_realloc() this can not happen, because the original pointer variable becomes immediately updated by:

*dest = realloc(*dest, new_size)

On the other hand, even with the original code you can provoke the problem with:

char *old = glob_prefs[0].svalue;
pref_init();
printf("%s", old);

This is guaranteed not to happen in the JPilot code, as you can see in main() of pilot.c, that glob_prefs[] never becomes accessed before pref_init(); in line 1665. After calling jp_pref_init(glob_prefs), all string values are no more NULL and point to malloc()-ed memory, that can be safely realloc()-ed by pref_lstrncpy_realloc().

I too have compiled my code with gcc -Wall and I don't get those warnings.