Would it be possible to post a complete reproducer? I don't fully understand the gnutls-server (I assume that is gnutls-serv binary) and gnutls-cli commands. And I am unable to replicate the issue with the just the instructions in the original report: Steps to Reproduce:
1. Run valgrind gnutls-server --echo
2. Connect using gnutls-cli

It would also be interesting to run valgrind --track-origins=yes to know where valgrind thinks the uninitialized memory is coming from.

The full command for the server is:
valgrind --track-origins=yes --error-exitcode=199 gnutls-serv --echo --port 2526 --x509keyfile srv.key --x509certfile srv.crt --x509cafile ca.crt

For the clients is:
gnutls-cli --x509cafile ca.crt --port 2526 localhost


The output from valgrind is:
==2344== Memcheck, a memory error detector
==2344== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==2344== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==2344== Command: gnutls-serv --echo --port 2526 --x509keyfile srv.key --x509certfile srv.crt --x509cafile ca.crt
==2344== 
Echo Server listening on 0.0.0.0 port 2526 family 2...done
Echo Server listening on :: port 2526 family 10...bind() failed: Address already in use
==2344== Conditional jump or move depends on uninitialised value(s)
==2344==    at 0x4BEEB09: _gcry_mpi_print (mpicoder.c:584)
==2344==    by 0x4BA2909: gcry_mpi_print (visibility.c:308)
==2344==    by 0x4D2CDD8: wrap_gcry_mpi_print (mpi-libgcrypt.c:76)
==2344==    by 0x4D299A0: _gnutls_dh_common_print_server_kx (auth_dh_common.c:326)
==2344==    by 0x4D215D9: gen_dhe_server_kx (auth_dhe.c:135)
==2344==    by 0x4D0D028: _gnutls_send_server_kx_message (gnutls_kx.c:200)
==2344==    by 0x4D09CEF: _gnutls_handshake_server (gnutls_handshake.c:2947)
==2344==    by 0x4D0A34A: gnutls_handshake (gnutls_handshake.c:2620)
==2344==    by 0x804CD0A: main (serv.c:1171)
==2344==  Uninitialised value was created by a stack allocation
==2344==    at 0x4D29921: _gnutls_dh_common_print_server_kx (auth_dh_common.c:310)
==2344== 
==2344== Conditional jump or move depends on uninitialised value(s)
==2344==    at 0x4BEEB09: _gcry_mpi_print (mpicoder.c:584)
==2344==    by 0x4BA2909: gcry_mpi_print (visibility.c:308)
==2344==    by 0x4D2CDD8: wrap_gcry_mpi_print (mpi-libgcrypt.c:76)
==2344==    by 0x4D299C9: _gnutls_dh_common_print_server_kx (auth_dh_common.c:327)
==2344==    by 0x4D215D9: gen_dhe_server_kx (auth_dhe.c:135)
==2344==    by 0x4D0D028: _gnutls_send_server_kx_message (gnutls_kx.c:200)
==2344==    by 0x4D09CEF: _gnutls_handshake_server (gnutls_handshake.c:2947)
==2344==    by 0x4D0A34A: gnutls_handshake (gnutls_handshake.c:2620)
==2344==    by 0x804CD0A: main (serv.c:1171)
==2344==  Uninitialised value was created by a stack allocation
==2344==    at 0x4D29921: _gnutls_dh_common_print_server_kx (auth_dh_common.c:310)
==2344== 
==2344== Conditional jump or move depends on uninitialised value(s)
==2344==    at 0x4BEEB09: _gcry_mpi_print (mpicoder.c:584)
==2344==    by 0x4BA2909: gcry_mpi_print (visibility.c:308)
==2344==    by 0x4D2CDD8: wrap_gcry_mpi_print (mpi-libgcrypt.c:76)
==2344==    by 0x4D299F2: _gnutls_dh_common_print_server_kx (auth_dh_common.c:328)
==2344==    by 0x4D215D9: gen_dhe_server_kx (auth_dhe.c:135)
==2344==    by 0x4D0D028: _gnutls_send_server_kx_message (gnutls_kx.c:200)
==2344==    by 0x4D09CEF: _gnutls_handshake_server (gnutls_handshake.c:2947)
==2344==    by 0x4D0A34A: gnutls_handshake (gnutls_handshake.c:2620)
==2344==    by 0x804CD0A: main (serv.c:1171)
==2344==  Uninitialised value was created by a stack allocation
==2344==    at 0x4D29921: _gnutls_dh_common_print_server_kx (auth_dh_common.c:310)
==2344== 
No certificates found!
received: Hello world

Exiting via signal 15
==2344== 
==2344== HEAP SUMMARY:
==2344==     in use at exit: 188,868 bytes in 1,698 blocks
==2344==   total heap usage: 9,242 allocs, 7,544 frees, 710,373 bytes allocated
==2344== 
==2344== LEAK SUMMARY:
==2344==    definitely lost: 0 bytes in 0 blocks
==2344==    indirectly lost: 0 bytes in 0 blocks
==2344==      possibly lost: 0 bytes in 0 blocks
==2344==    still reachable: 188,868 bytes in 1,698 blocks
==2344==         suppressed: 0 bytes in 0 blocks
==2344== Rerun with --leak-check=full to see details of leaked memory
==2344== 
==2344== For counts of detected and suppressed errors, rerun with: -v
==2344== ERROR SUMMARY: 3 errors from 3 contexts (suppressed: 19 from 8)

Thanks for the reproducer and for the --track-origins=yes run output.

valgrind is complaining about the following:

- auth_dh_common.c:

int
_gnutls_dh_common_print_server_kx (gnutls_session_t session,
                                   bigint_t g, bigint_t p, opaque ** data,
                                   int psk)
{
  bigint_t x, X;
  size_t n_X, n_g, n_p;
  int ret, data_size, pos;
  uint8_t *pdata;

  X = gnutls_calc_dh_secret (&x, g, p);
  if (X == NULL || x == NULL)
    {
      gnutls_assert ();
      return GNUTLS_E_MEMORY_ERROR;
    }

  session->key->dh_secret = x;
  _gnutls_dh_set_secret_bits (session, _gnutls_mpi_get_nbits (x));

  _gnutls_mpi_print (g, NULL, &n_g);
  _gnutls_mpi_print (p, NULL, &n_p);
  _gnutls_mpi_print (X, NULL, &n_X);

The above three lines (auth_dh_common.c:326-328) is where the complains originate. As you can see it provides pointers to the uninitialized values of n_g, n_p and n_X.

#define _gnutls_mpi_print(x,y,z) _gnutls_mpi_ops.bigint_print(x,y,z,GNUTLS_MPI_FORMAT_USG)
.bigint_print = wrap_gcry_mpi_print

- mpi-libgcrypt.c:

static int
wrap_gcry_mpi_print (const bigint_t a, void *buffer, size_t * nbytes,
                     gnutls_bigint_format_t format)
{
  int ret;

  format = _format_conv (format);

  if (nbytes == NULL || a == NULL)
    return GNUTLS_E_INVALID_REQUEST;

  ret = gcry_mpi_print (format, buffer, *nbytes, nbytes, a);

So it passes a NULL buffer and nbytes twice (once as actual value, ones as pointer).

- visibility.c:

gcry_error_t
gcry_mpi_print (enum gcry_mpi_format format,
                unsigned char *buffer, size_t buflen,
                size_t *nwritten,
                const gcry_mpi_t a)
{
  return _gcry_mpi_print (format, buffer, buflen, nwritten, a);
}

gcry_error_t
gcry_mpi_print (enum gcry_mpi_format format,
                unsigned char *buffer, size_t buflen,
                size_t *nwritten, struct gcry_mpi *a)
{
  unsigned int nbits = mpi_get_nbits (a);
  size_t len;
  size_t dummy_nwritten;

  if (!nwritten)
    nwritten = &dummy_nwritten;

  len = buflen;
  *nwritten = 0;
[...]

So here len is assigned the uninitialized bytes and:

  else if (format == GCRYMPI_FMT_USG)
    {
      unsigned int n = (nbits + 7)/8;

      /* Note:  We ignore the sign for this format.  */
      /* FIXME: for performance reasons we should put this into
         mpi_aprint because we can then use the buffer directly.  */
      if (buffer && n > len)
        return gcry_error (GPG_ERR_TOO_SHORT);

Here (mpicoder.c:584) it is tested against n, if buffer != NULL. But buffer is NULL. That is indeed a little surprising.

> Here (mpicoder.c:584) it is tested against n, if buffer != NULL.
> But buffer is NULL. That is indeed a little surprising.

It does seem to be how GCC compiles this code though:

(gdb) disassemble /m _gcry_mpi_print
Dump of assembler code for function _gcry_mpi_print:
[...]
578	    {
579	      unsigned int n = (nbits + 7)/8;
   0x0064faf8 <+120>:	lea    0x7(%eax),%eax
   0x0064fafe <+126>:	shr    $0x3,%eax
   0x0064fb11 <+145>:	mov    %eax,-0x20(%ebp)

580	
581	      /* Note:  We ignore the sign for this format.  */
582	      /* FIXME: for performance reasons we should put this into
583		 mpi_aprint because we can then use the buffer directly.  */
584	      if (buffer && n > len)
   0x0064fafb <+123>:	mov    0xc(%ebp),%edx
   0x0064fb01 <+129>:	test   %edx,%edx
   0x0064fb03 <+131>:	setne  %dl
   0x0064fb06 <+134>:	cmp    %eax,0x10(%ebp)
   0x0064fb09 <+137>:	jb     0x64fc18 <_gcry_mpi_print+408>
   0x0064fc18 <+408>:	test   %dl,%dl
   0x0064fc1a <+410>:	je     0x64fb0f <_gcry_mpi_print+143>

585	        return gcry_error (GPG_ERR_TOO_SHORT);
586	      if (buffer)
   0x0064fb0f <+143>:	test   %dl,%dl
   0x0064fb14 <+148>:	je     0x64fb5b <_gcry_mpi_print+219>

I don't see how we can teach valgrind about this so we'll need a suppression for it. Put the following into some file (call it gnutls.supp)

{
   gnutls_gen_dh_gcry_mpi_print_suppression
   Memcheck:Cond
   fun:_gcry_mpi_print
   fun:gcry_mpi_print
   fun:wrap_gcry_mpi_print
   fun:_gnutls_gen_dh_common_client_kx
}

Then run with valgrind --suppressions=gnutls.supp --error-exitcode=199 gnutls-serv --echo --port 2526 --x509keyfile srv.key --x509certfile srv.crt --x509cafile ca.crt

Would that be a workable workaround for you?

> Would that be a workable workaround for you?

Well, I can modify my test case to have a custom suppression, sure.

Small error though (server vs client):
{
    gnutls_gen_dh_gcry_mpi_print_suppression
    Memcheck:Cond
    fun:_gcry_mpi_print
    fun:gcry_mpi_print
    fun:wrap_gcry_mpi_print
    fun:_gnutls_dh_common_print_server_kx
}


But as gnutls is part of standard RHEL release, is a library, and this issue is caused by RHEL specific compile flags, shouldn't we ship this suppression in valgrind package by default?

You know, so that developers wouldn't have to recompile whole gnutls without optimisations just to rule this warnings as false positives.

I checked all architectures and the warning is present only on i386. As it's the only 32 bit arch we ship, it's probable that it's the only one in which gcc uses this (significantly) different optimisation approach.

I'll be testing -O0 build tomorrow to see if it makes it go away.

If you want to talk to the package owner contact Tomas Mraz (tmraz), he already knows about this.

If you provide preprocessed source + gcc options, we could look why gcc does this.

@Jakub:

OK, I'll try to get the preprocessor output.

I've re run the test with gcrypt compiled with -O0 and the warning went away so it looks like effect of gcc optimisation:

==28761== Memcheck, a memory error detector
==28761== Copyright (C) 2002-2012, and GNU GPL'd, by Julian Seward et al.
==28761== Using Valgrind-3.8.1 and LibVEX; rerun with -h for copyright info
==28761== Command: gnutls-serv --echo --port 2526 --x509keyfile srv.key --x509certfile srv.crt --x509cafile ca.crt
==28761== 
Echo Server listening on 0.0.0.0 port 2526 family 2...done
Echo Server listening on :: port 2526 family 10...bind() failed: Address already in use
No certificates found!
received: Hello world

Exiting via signal 15
==28761== 
==28761== HEAP SUMMARY:
==28761==     in use at exit: 188,868 bytes in 1,698 blocks
==28761==   total heap usage: 9,326 allocs, 7,628 frees, 716,000 bytes allocated
==28761== 
==28761== LEAK SUMMARY:
==28761==    definitely lost: 0 bytes in 0 blocks
==28761==    indirectly lost: 0 bytes in 0 blocks
==28761==      possibly lost: 0 bytes in 0 blocks
==28761==    still reachable: 188,868 bytes in 1,698 blocks
==28761==         suppressed: 0 bytes in 0 blocks
==28761== Rerun with --leak-check=full to see details of leaked memory
==28761== 
==28761== For counts of detected and suppressed errors, rerun with: -v
==28761== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 19 from 8)

The preprocessor output is no different between -O0 and -O3 build:

  else if (format == GCRYMPI_FMT_USG)
    {
      unsigned int n = (nbits + 7)/8;




      if (buffer && n > len)
        return gcry_error (GPG_ERR_TOO_SHORT);
      if (buffer)
        {
          unsigned char *tmp;

          tmp = _gcry_mpi_get_buffer (a, &n, ((void *)0));
          if (!tmp)
            return gpg_error_from_syserror ();
          memcpy (buffer, tmp, n);

The options used for building are standard rhel cflags:
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4

But the above is just a small portion of the preprocessor output, so nothing I can run compiler on.

Created attachment 762865 [details]
-O2 preprocessed mpicoder.c

full mpicoder file after running through preprocessor with -O2 flag

Created attachment 762866 [details]
-O2 assembly mpicoder.c

assembler output of mpicoder.c with -O2 CFLAGS

Created attachment 762867 [details]
-O0 preprocessed mpicoder.c

mpicoder.c preprocessed with -O0 flags

Created attachment 762868 [details]
-O0 assembly mpicoder.c

assembly version of mpicoder.c compiled with -O2 flag

I'd suggest just to deobfuscate the code, instead of doing:

      if (buffer && n > len)
        return gcry_error (GPG_ERR_TOO_SHORT);
      if (buffer)
        {
          unsigned char *tmp;

          tmp = _gcry_mpi_get_buffer (a, &n, ((void *)0));
...

do:
      if (buffer)
        {
          unsigned char *tmp;

          if (n > len)
            return gcry_error (GPG_ERR_TOO_SHORT);
          tmp = _gcry_mpi_get_buffer (a, &n, ((void *)0));
then I think you won't see valgrind warnings.
What happens is because n > len doesn't have side-effects that gcc optimizes it actually as if ((buffer != NULL) & (n > len)) (though, in this case it in the end actually doesn't turn out to be a good optimization).

Filed http://gcc.gnu.org/PR57650 to track possible code generation improvements though.

I wouldn't say the code is particularly obfuscated. As this pattern appears on multiple places in the mpicoder.c I am afraid that upstream might not be too fancy to change it just to suppress the valgrind warnings.

(In reply to Tomas Mraz from comment #20)
> I wouldn't say the code is particularly obfuscated. As this pattern appears
> on multiple places in the mpicoder.c I am afraid that upstream might not be
> too fancy to change it just to suppress the valgrind warnings.

I do think the code would be a bit more readable if written in the more obvious way as suggested in comment #19. It would not just be for readability or to suppress the valgrind warnings, but also for creating more efficient code because of the GCC missed optimisation (in that way the valgrind warnings are correct, the code does an unneeded and unused test against possibly uninitialized values).

If possible I rather see the package include a fix (workaround for the gcc codegen) than add an extra default suppression. But for now we should recommend people use the suppression themselves from comment #8 and/or comment #9 (you might need both).

I don't think this is a real valgrind issue, reassigning to gnutls.

I am no longer able to reproduce the issue with following package versions:

gnutls-2.8.5-19.el6_7.i686
gnutls-2.8.5-19.el6_7.i686
gnutls-utils-2.8.5-19.el6_7.i686
libgcrypt-1.4.5-12.el6_8.i686
valgrind-3.8.1-8.el6.i686

closing as not relevant any more.