[neomutt-devel] Bogus pointer arithmetic

[Alejandro Colomar]

Hi Rich,

While building from source, I noticed a warning that was weird:

	$ make
	cc -Werror	 -fno-delete-null-pointer-checks -D_ALL_SOURCE=1 -D_GNU_SOURCE=1 -D__EXTENSIONS__ -D_XOPEN_SOURCE_EXTENDED -I/usr/include/lua5.4 -I/usr/include -I/usr/include -DNCURSES_WIDECHAR -I/usr/include/p11-kit-1 -I/usr/include -isystem /usr/include/mit-krb5 -O2 -I. -I. -Wall  -I./test -MT send/smtp.o -MD -MP -MF send/smtp.Tpo -c -o send/smtp.o send/smtp.c
	In function ‘smtp_get_auth_response’,
	    inlined from ‘smtp_auth_gsasl’ at send/smtp.c:569:9:
	send/smtp.c:496:9: error: array subscript 3 is outside array bounds of ‘char[1]’ [-Werror=array-bounds=]
	  496 |     if (*smtp_response)
	      |         ^~~~~~~~~~~~~~
	send/smtp.c:496:9: error: array subscript 3 is outside array bounds of ‘char[1]’ [-Werror=array-bounds=]
	cc1: all warnings being treated as errors
	make: *** [Makefile:722: send/smtp.o] Error 1

I first thought it would be an old flexible array member, and checked if
I could modify it to use the new syntax, '[]'.  But then I realized it
looks like a bug of a completely different kind.

	$ grepc -tuf '\bsmtp_response\b' send/smtp.c
	send/smtp.c:479:
	static int smtp_get_auth_response(struct Connection *conn, struct Buffer *input_buf,
					  int *smtp_rc, struct Buffer *response_buf)
	{
	  buf_reset(response_buf);
	  do
	  {
	    if (mutt_socket_buffer_readln(input_buf, conn) < 0)
	      return -1;
	    if (!smtp_code(buf_string(input_buf), buf_len(input_buf) + 1 /* number of bytes */, smtp_rc))
	    {
	      return -1;
	    }

	    if (*smtp_rc != SMTP_READY)
	      break;

	    const char *smtp_response = buf_string(input_buf) + 3;
	    if (*smtp_response)
	    {
	      smtp_response++;
	      buf_addstr(response_buf, smtp_response);
	    }
	  } while (buf_string(input_buf)[3] == '-');

	  return 0;
	}

Let's see what this buf_string() gives.

	$ grepc buf_string
	./mutt/buffer.h:93:
	static inline const char *buf_string(const struct Buffer *buf)
	{
	  if (!buf || !buf->data)
	    return "";

	  return buf->data;
	}

Huh, in case `!buf->data`, this function returns "".  In that case, the
`+ 3` is already Undefined Behavior, since it's a pointer to 3 after the
end of the array (the maximum legal one is 1 after the end).  And not
only that, but we're actually dereferencing it.

Or do you know for sure that `!buf->data` will never happen in this
specific call?

Cheers,
Alex

-- 
<https://www.alejandro-colomar.es/>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://mailman.neomutt.org/pipermail/neomutt-devel-neomutt.org/attachments/20231026/c6c50cb5/attachment.sig>