Logo Questions Linux Laravel Mysql Ubuntu Git Menu
 

sem_open - valgrind complains about uninitialised bytes

I have a trivial program:

int main(void)
{
  const char sname[]="xxx";
  sem_t *pSemaphor;
  if ((pSemaphor = sem_open(sname, O_CREAT, 0644, 0)) == SEM_FAILED) {
    perror("semaphore initilization");
    exit(1);
  }
  sem_unlink(sname);
  sem_close(pSemaphor);
}

When I run it under valgrind, I get the following error:

==12702== Syscall param write(buf) points to uninitialised byte(s)
==12702==    at 0x4E457A0: __write_nocancel (syscall-template.S:81)
==12702==    by 0x4E446FC: sem_open (sem_open.c:245)
==12702==    by 0x4007D0: main (test.cpp:15)
==12702==  Address 0xfff00023c is on thread 1's stack
==12702==  in frame #1, created by sem_open (sem_open.c:139)

The code was extracted from a bigger project where it ran successfully for years, but now it is causing segmentation fault.

The valgrind error from my example is the same as seen in the bigger project, but there it causes a crash, which my small example doesn't.

like image 720
user2194898 Avatar asked Feb 23 '16 12:02

user2194898


1 Answers

I see this with glibc 2.27-5 on Debian. In my case I only open the semaphores right at the start of a long-running program and it seems harmless so far - just annoying.

Looking at the code for sem_open.c which is available at: https://code.woboq.org/userspace/glibc/nptl/sem_open.c.html

It seems that valgrind is complaining about the line (270 as I look now):

if (TEMP_FAILURE_RETRY (__libc_write (fd, &sem.initsem, sizeof (sem_t)))
      == sizeof (sem_t)

However sem.initsem is properly initialised earlier in a fairly baroque manner, firstly by explicitly setting fields in the sem.newsem (part of the union), and then once that is done by a call to memset (L226-228):

  /* Initialize the remaining bytes as well.  */
  memset ((char *) &sem.initsem + sizeof (struct new_sem), '\0',
          sizeof (sem_t) - sizeof (struct new_sem));

I think that this particular shenanigans is all quite optimal, but we need to make sure that all of the fields of new_sem have actually been initialised... we find the definition in https://code.woboq.org/userspace/glibc/sysdeps/nptl/internaltypes.h.html and it is this wonderful creation:

struct new_sem
{
#if __HAVE_64B_ATOMICS
  /* The data field holds both value (in the least-significant 32 bytes) and
     nwaiters.  */
# if __BYTE_ORDER == __LITTLE_ENDIAN
#  define SEM_VALUE_OFFSET 0
# elif __BYTE_ORDER == __BIG_ENDIAN
#  define SEM_VALUE_OFFSET 1
# else
# error Unsupported byte order.
# endif
# define SEM_NWAITERS_SHIFT 32
# define SEM_VALUE_MASK (~(unsigned int)0)
  uint64_t data;
  int private;
  int pad;
#else
# define SEM_VALUE_SHIFT 1
# define SEM_NWAITERS_MASK ((unsigned int)1)
  unsigned int value;
  int private;
  int pad;
  unsigned int nwaiters;
#endif
};

So if we __HAVE_64B_ATOMICS then the structure has a data field which contains both the value and the nwaiters, otherwise these are separate fields.

In the initialisation of sem.newsem we can see that these are initialised correctly, as follows:

#if __HAVE_64B_ATOMICS
      sem.newsem.data = value;
#else
      sem.newsem.value = value << SEM_VALUE_SHIFT;
      sem.newsem.nwaiters = 0;
#endif
      /* pad is used as a mutex on pre-v9 sparc and ignored otherwise.  */
      sem.newsem.pad = 0;
      /* This always is a shared semaphore.  */
      sem.newsem.private = FUTEX_SHARED;

I'm doing all of this on a 64-bit system, so I think that valgrind is complaining about the initialisation of the 64-bit sem.newsem.data with a 32-bit value since from:

  value = va_arg (ap, unsigned int);

We can see that value is defined simply as an unsigned int which will usually still be 32 bits even on a 64-bit system (see What should be the sizeof(int) on a 64-bit machine?), but that should just be an implicit cast to 64-bits when it is assigned.

So I think this is not a bug - just valgrind getting confused.

like image 81
karora Avatar answered Oct 21 '22 02:10

karora