GCC warning about acessing bytes at offset: what is GCC complaining about?

Viewed 842

I have an old C library that I have to build on a modern/current system. This library has not been developed for at least 5 or 6 years (perhaps even more). So naturally, building it with a modern C compiler throws a lot of warnings. So I started going through the warnings and fixing the code.

But there is one warning where I really don't know what it means. I'm compiling with GCC 9.3.0 and -O2 -m32 -Wall in the CFLAGS:

[ 17%] Building C object cu_datastructs/CMakeFiles/cu_datastructs.dir/cu_nlist.c.o
In file included from /usr/include/string.h:519,
                 from /home/shaoran/projects/cu_utils/remus_installer/cu_utils/cu_datastructs/cu_nlist.c:3:
In function ‘strcpy’,
    inlined from ‘cu_nlist_swap’ at /home/shaoran/projects/cu_utils/remus_installer/cu_utils/cu_datastructs/cu_nlist.c:389:2:
/usr/include/bits/string_fortified.h:90:10: warning: ‘__builtin_strcpy’ accessing 1 byte at offsets [-1073741824, 1073741823] and [-1073741824, 1073741823] may overlap 1 byte at offset -1073741824 [-Wrestrict]
   90 |   return __builtin___strcpy_chk (__dest, __src, __bos (__dest));
      |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The code in question is:

int cu_nlist_swap(struct cu_nlist *nl, int id1, int id2)
{
    unsigned char *c_tmp;
    cu_nlist_names n_tmp;

    if(nl == NULL || id1 < 0 || id2 < 0)
        return 0;

    if(id1 >= nl->size || id2 >= nl->size) /* out of bounds */
        return 0;

    if(id1 == id2)
        return 1;

    c_tmp = nl->items[id1];
    nl->items[id1] = nl->items[id2];
    nl->items[id2] = c_tmp;

    strcpy(n_tmp, nl->names[id1]);
    strcpy(nl->names[id1], nl->names[id2]);   // <--- line with the warning
    strcpy(nl->names[id2], n_tmp);

    return 1;
}

where cu_nlist is defined as:

// on a different .h file
#define CU_NAMELEN 1024

// ...

#define CU_NLIST_NAME_LEN   CU_NAMELEN
#define CU_NLIST_STARTCAP   2

typedef unsigned char*  (*create_f)();
typedef void    (*free_f)(unsigned char*);
typedef int     (*copy_f)(unsigned char*,unsigned char*);

typedef char    cu_nlist_names[CU_NLIST_NAME_LEN];

typedef struct cu_nlist {
    create_f         ocreate;
    free_f           ofree;
    copy_f           ocopy;
    int              cap;
    int              size;
    unsigned char**  items;  //< holds the items in the named list, initialized with NULL
    cu_nlist_names*  names;  //< holds the names for the items, initialized with NULL
} cu_nlist;

Am I blind? Why is the cu_nlist_swap generating this warning, and what does that mean? If I remove -O2 then the warning is not issued, I cannot understand it. What exactly is GCC telling me?

So, the idea of this structure is to have a named list of objects, when you append a new value, the append function checks the size. If the number of elements (size) reached the maximum (cap), then we resize the buffers to hold double the amount of the previous memory. This is done via this function:

static  int nlist_resize(struct cu_nlist* nl)
{
    void *tmp;
    int sz;
    if (nl->cap == 0)
        sz = CU_NLIST_STARTCAP;
    else
        sz = 2*nl->cap;

    tmp = realloc(nl->items, sz * sizeof(unsigned char*));
    if(tmp == NULL)
        return 0;
    nl->items = tmp;

    tmp = realloc(nl->names, sz*sizeof(cu_nlist_names));
    if(tmp == NULL)
        return 0;
    nl->names = tmp;

    nl->cap = sz;
    return 1;
}

So nl->names points to a memory block of 1024*sz bytes, nl->names[0] points to the first 1024 bytes, nl->names[1] points to the next 1024 bytes, etc. So I don't see why there may be memory that overlaps. What am I missing here?

I know that declaring name as char** name and allocating CU_NLIST_NAME_LEN bytes for every new name when cu_nlist_append is called would be better, but I would have to rewrite a lot more of this code and this is something that at the moment I cannot do. I know this is not pretty code and could be more efficient with the memory usage, but I didn't write this code, I'm just maintaining it (it's legacy code from old projects of my company that are not developed anymore). I want to understand what GCC is warning me about.

1 Answers

One of the requirements of strcpy() is that source and destination must not overlap. Unfortunately, when doing its work, it also doesn't care how you've defined your buffers in memory. It continues to plough on until it encounters a zero terminator. Hence when copying from one adjacent buffer to another, it could end up overrunning if the terminating zero is not found. It might be that the inlining in -O2 somehow exposes this threat when your buffers are adjacent (as members in nl->names[] are).

Anyway, I'd switch to strncpy() - you know your buffer lengths, it would be a trivial change and might shut gcc up.

Related