[Qemu-devel] [PATCH] security_20040618

[Qemu-devel] [PATCH] security_20040618

[Tim]

[-- Attachment #1: Type: text/plain, Size: 413 bytes --]

Updates to this, from the previous version two days ago, include
additional strcpy replacements, as well as TODO comments pointing out
unhealthy calls to {m,re}alloc, which don't check for NULL return
values. (I am not sure how to handle error return codes in most places,
so just comments for now.)  Also pstrcpy() in vl.c was simplified, but
should behave exactly the same as before.

See attached.
Thanks,
tim

[-- Attachment #2: README --]
[-- Type: text/plain, Size: 1117 bytes --]

This patch contains fixes for calls to potentially unsafe string
manipulation functions.  It is almost certain that some of these updates
patch security vulnerabilities.  Other changes may avert future
vulnerabilities.  In addition, TODO comments were added to certain
sections of code where further checks should be implemented.  This
version of the patch makes changes to the following files in the most
current CVS (as of today): 

block.c
dyngen.c
monitor.c
sparc-dis.c
thunk.c
vl.c
linux-user/elfload.c
linux-user/path.c
linux-user/syscall.c
slirp/debug.c
slirp/mbuf.c
slirp/misc.c
slirp/tcp_subr.c
slirp/udp.c
target-i386/helper2.c

Any original code contained within this patch is hereby released into
the public domain.  All derivative portions of code contained within,
for the purposes of documentation, are a part of QEMU, and are therefore
protected by the copyright of their respective authors.

While care was taken in making these changes, it is possible that this
patch will break something.  Use your own risk.  Please report any
success/failure to:
tim {HYPHEN} qemu {AT} sentinelchicken {DOT} org.

[-- Attachment #3: security.patch --]
[-- Type: text/plain, Size: 13175 bytes --]

--- qemu-current/block.c	2004-06-16 20:49:58.000000000 -0700
+++ qemu-dev/block.c	2004-06-17 22:14:20.000000000 -0700
@@ -92,7 +92,7 @@
     bs->fd = -1;
     bs->cow_fd = -1;
     bs->cow_bitmap = NULL;
-    strcpy(bs->filename, filename);
+    snprintf(bs->filename, 1024, "%s", filename);
 
     /* open standard HD image */
 #ifdef _WIN32
--- qemu-current/dyngen.c	2004-04-04 05:56:28.000000000 -0700
+++ qemu-dev/dyngen.c	2004-06-18 08:33:58.000000000 -0700
@@ -458,6 +458,7 @@
 
     /* read all section data */
     sdata = malloc(sizeof(void *) * ehdr.e_shnum);
+    /* TODO: check for sdata == NULL */
     memset(sdata, 0, sizeof(void *) * ehdr.e_shnum);
     
     for(i = 0;i < ehdr.e_shnum; i++) {
@@ -653,6 +654,7 @@
 	
     /* read all section data */
     sdata = malloc(sizeof(void *) * fhdr.f_nscns);
+    /* TODO: check for sdata == NULL */
     memset(sdata, 0, sizeof(void *) * fhdr.f_nscns);
     
     const char *p;
@@ -698,6 +700,7 @@
 
 	/* set coff symbol */
 	symtab = malloc(sizeof(struct coff_sym) * nb_syms);
+        /* TODO: check for symtab == NULL */
 
 	int aux_size, j;
 	for (i = 0, ext_sym = coff_symtab, sym = symtab; i < nb_syms; i++, ext_sym++, sym++) {
@@ -746,6 +749,7 @@
 
     /* set coff relocation */
     relocs = malloc(sizeof(struct coff_rel) * nb_relocs);
+    /* TODO: check for relocs == NULL */
     for (i = 0, ext_rel = coff_relocs, rel = relocs; i < nb_relocs; 
          i++, ext_rel++, rel++) {
         memset(rel, 0, sizeof(*rel));
--- qemu-current/monitor.c	2004-06-16 20:49:59.000000000 -0700
+++ qemu-dev/monitor.c	2004-06-17 22:12:49.000000000 -0700
@@ -1221,7 +1221,7 @@
                     goto fail;
                 }
                 str = qemu_malloc(strlen(buf) + 1);
-                strcpy(str, buf);
+                pstrcpy(str, strlen(buf) + 1,  buf);
                 str_allocated[nb_args] = str;
             add_str:
                 if (nb_args >= MAX_ARGS) {
@@ -1437,7 +1437,7 @@
 static void term_print_cmdline (const char *cmdline)
 {
     term_show_prompt();
-    term_printf(cmdline);
+    term_printf("%s", cmdline);
     term_flush();
 }
 
@@ -1521,7 +1521,7 @@
     }
     term_hist_entry--;
     if (term_hist_entry >= 0) {
-	strcpy(term_cmd_buf, term_history[term_hist_entry]);
+	snprintf(term_cmd_buf, 4096, "%s", term_history[term_hist_entry]);
 	term_printf("\n");
 	term_print_cmdline(term_cmd_buf);
 	term_cmd_buf_index = term_cmd_buf_size = strlen(term_cmd_buf);
@@ -1533,7 +1533,7 @@
     if (term_hist_entry == TERM_MAX_CMDS - 1 || term_hist_entry == -1)
 	return;
     if (term_history[++term_hist_entry] != NULL) {
-	strcpy(term_cmd_buf, term_history[term_hist_entry]);
+	snprintf(term_cmd_buf, 4096, "%s", term_history[term_hist_entry]);
     } else {
 	term_hist_entry = -1;
     }
--- qemu-current/sparc-dis.c	2003-06-09 08:23:31.000000000 -0700
+++ qemu-dev/sparc-dis.c	2004-06-18 09:05:50.000000000 -0700
@@ -2468,6 +2468,7 @@
       if (!opcodes_initialized)
 	sorted_opcodes = (const struct sparc_opcode **)
             malloc (sparc_num_opcodes * sizeof (struct sparc_opcode *));
+      /* TODO: check for sorted_opcodes == NULL */
       /* Reset the sorted table so we can resort it.  */
       for (i = 0; i < sparc_num_opcodes; ++i)
 	sorted_opcodes[i] = &sparc_opcodes[i];
@@ -3183,6 +3184,7 @@
   if (hash_buf != NULL)
     free (hash_buf);
   hash_buf = (struct opcode_hash *) malloc (sizeof (struct opcode_hash) * num_opcodes);
+  /* TODO: check for hash_buf == NULL */
   for (i = num_opcodes - 1; i >= 0; --i)
     {
       register int hash = HASH_INSN (opcode_table[i]->match);
--- qemu-current/thunk.c	2003-06-15 12:52:54.000000000 -0700
+++ qemu-dev/thunk.c	2004-06-18 08:48:05.000000000 -0700
@@ -85,6 +85,7 @@
         offset = 0;
         max_align = 1;
         se->field_offsets[i] = malloc(nb_fields * sizeof(int));
+        /* TODO: check for NULL from malloc() */
         type_ptr = se->field_types;
         for(j = 0;j < nb_fields; j++) {
             size = thunk_type_size(type_ptr, i);
--- qemu-current/vl.c	2004-06-05 06:46:47.000000000 -0700
+++ qemu-dev/vl.c	2004-06-18 08:58:21.000000000 -0700
@@ -265,19 +265,11 @@
 
 void pstrcpy(char *buf, int buf_size, const char *str)
 {
-    int c;
-    char *q = buf;
-
-    if (buf_size <= 0)
-        return;
-
-    for(;;) {
-        c = *str++;
-        if (c == 0 || q >= buf + buf_size - 1)
-            break;
-        *q++ = c;
+    if(buf_size > 0) {
+        buf_size--;
+        strncpy(buf, str, buf_size);
+        buf[buf_size] = '\0';
     }
-    *q = '\0';
 }
 
 /* strcat and truncate. */
--- qemu-current/linux-user/elfload.c	2004-06-16 20:47:28.000000000 -0700
+++ qemu-dev/linux-user/elfload.c	2004-06-15 19:31:55.000000000 -0700
@@ -1067,7 +1067,7 @@
 	char * passed_p;
 
 	if (interpreter_type == INTERPRETER_AOUT) {
-	    sprintf(passed_fileno, "%d", bprm->fd);
+	    snprintf(passed_fileno, sizeof(passed_fileno), "%d", bprm->fd);
 	    passed_p = passed_fileno;
 
 	    if (elf_interpreter) {
--- qemu-current/linux-user/path.c	2003-04-10 17:15:13.000000000 -0700
+++ qemu-dev/linux-user/path.c	2004-06-18 11:02:32.000000000 -0700
@@ -43,7 +43,9 @@
 				  struct pathelem *parent,
 				  const char *name)
 {
+    /* TODO: is this line safe? */
     struct pathelem *new = malloc(sizeof(*new));
+    /* TODO: check for new == NULL */
     new->name = strdup(name);
     asprintf(&new->pathname, "%s/%s", root, name);
     new->num_entries = 0;
@@ -75,7 +77,7 @@
 
     root = realloc(root, sizeof(*root)
 		   + sizeof(root->entries[0])*root->num_entries);
-
+    /* TODO: check for root == NULL */
     root->entries[root->num_entries-1] = new_entry(root->pathname, root, name);
     root->entries[root->num_entries-1]
 	= add_dir_maybe(root->entries[root->num_entries-1]);
--- qemu-current/linux-user/syscall.c	2004-06-16 20:43:10.000000000 -0700
+++ qemu-dev/linux-user/syscall.c	2004-06-18 09:03:45.000000000 -0700
@@ -1409,6 +1409,7 @@
     
     if (flags & CLONE_VM) {
         ts = malloc(sizeof(TaskState) + NEW_STACK_SIZE);
+        /* TODO: check for ts == NULL */
         memset(ts, 0, sizeof(TaskState));
         new_stack = ts->stack;
         ts->used = 1;
@@ -2450,7 +2451,7 @@
 		    tnamelen = treclen - (2 * sizeof(target_long) + 2);
 		    if (tnamelen > 256)
                         tnamelen = 256;
-		    strncpy(tde->d_name, de->d_name, tnamelen);
+		    snprintf(tde->d_name, tnamelen, '%s', de->d_name);
                     de = (struct dirent *)((char *)de + reclen);
                     len -= reclen;
                     tde = (struct dirent *)((char *)tde + treclen);
--- qemu-current/slirp/debug.c	2004-06-16 20:43:10.000000000 -0700
+++ qemu-dev/slirp/debug.c	2004-06-15 19:40:42.000000000 -0700
@@ -305,7 +305,7 @@
 			
 	for (so = tcb.so_next; so != &tcb; so = so->so_next) {
 		
-		n = sprintf(buff, "tcp[%s]", so->so_tcpcb?tcpstates[so->so_tcpcb->t_state]:"NONE");
+		n = snprintf(buff, sizeof(buff),"tcp[%s]", so->so_tcpcb?tcpstates[so->so_tcpcb->t_state]:"NONE");
 		while (n < 17)
 		   buff[n++] = ' ';
 		buff[17] = 0;
@@ -319,7 +319,7 @@
 		   
 	for (so = udb.so_next; so != &udb; so = so->so_next) {
 		
-		n = sprintf(buff, "udp[%d sec]", (so->so_expire - curtime) / 1000);
+		n = snprintf(buff, sizeof(buff), "udp[%d sec]", (so->so_expire - curtime) / 1000);
 		while (n < 17)
 		   buff[n++] = ' ';
 		buff[17] = 0;
--- qemu-current/slirp/mbuf.c	2004-04-21 17:10:47.000000000 -0700
+++ qemu-dev/slirp/mbuf.c	2004-06-18 11:04:03.000000000 -0700
@@ -152,6 +152,7 @@
         if (m->m_flags & M_EXT) {
 	  /* datasize = m->m_data - m->m_ext; */
 	  m->m_ext = (char *)realloc(m->m_ext,size);
+	  /* TODO: fix? and re-add check for m->m_ext == NULL */
 /*		if (m->m_ext == NULL)
  *			return (struct mbuf *)NULL;
  */		
@@ -161,6 +162,7 @@
 	  char *dat;
 	  datasize = m->m_data - m->m_dat;
 	  dat = (char *)malloc(size);
+          /* TODO: fix? and re-add the dat==NULL check */
 /*		if (dat == NULL)
  *			return (struct mbuf *)NULL;
  */
--- qemu-current/slirp/misc.c	2004-06-16 20:43:10.000000000 -0700
+++ qemu-dev/slirp/misc.c	2004-06-18 11:04:55.000000000 -0700
@@ -182,6 +182,7 @@
 	
 	tmp_ptr = *ex_ptr;
 	*ex_ptr = (struct ex_list *)malloc(sizeof(struct ex_list));
+	/* TODO: check for *ex_ptr == NULL */
 	(*ex_ptr)->ex_fport = port;
 	(*ex_ptr)->ex_addr = addr;
 	(*ex_ptr)->ex_pty = do_pty;
@@ -362,10 +363,10 @@
 		
 		if (x_port >= 0) {
 #ifdef HAVE_SETENV
-			sprintf(buff, "%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
+			snprintf(buff, sizeof(buff), "%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
 			setenv("DISPLAY", buff, 1);
 #else
-			sprintf(buff, "DISPLAY=%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
+			snprintf(buff, sizeof(buff), "DISPLAY=%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
 			putenv(buff);
 #endif
 		}
@@ -401,7 +402,7 @@
 		  {
 			  char buff[256];
 			  
-			  sprintf(buff, "Error: execvp of %s failed: %s\n", 
+			  snprintf(buff, sizeof(buff), "Error: execvp of %s failed: %s\n", 
 				  argv[0], strerror(errno));
 			  write(2, buff, strlen(buff)+1);
 		  }
@@ -448,6 +449,7 @@
 	char *bptr;
 	
 	bptr = (char *)malloc(strlen(str)+1);
+	/* TODO: check for bptr == NULL */
 	strcpy(bptr, str);
 	
 	return bptr;
@@ -476,7 +478,7 @@
 		sock_in.sin_port = htons(slirp_socket_port);
 		if (connect(s, (struct sockaddr *)&sock_in, sizeof(sock_in)) != 0)
 		   slirp_exit(1); /* just exit...*/
-		sprintf(buff, "kill %s:%d", slirp_socket_passwd, slirp_socket_unit);
+		snprintf(buff, sizeof(buff), "kill %s:%d", slirp_socket_passwd, slirp_socket_unit);
 		write(s, buff, strlen(buff)+1);
 	}
 #ifndef NO_UNIX_SOCKETS
@@ -489,7 +491,7 @@
 		if (connect(s, (struct sockaddr *)&sock_un,
 			      sizeof(sock_un.sun_family) + sizeof(sock_un.sun_path)) != 0)
 		   slirp_exit(1);
-		sprintf(buff, "kill none:%d", slirp_socket_unit);
+		snprintf(buff, sizeof(buff), "kill none:%d", slirp_socket_unit);
 		write(s, buff, strlen(buff)+1);
 	}
 #endif
@@ -615,7 +617,7 @@
 			                        
 			lprint_sb->sb_data = (char *)realloc(lprint_sb->sb_data,
 							     lprint_sb->sb_datalen + TCP_SNDSPACE);
-			
+			/* TODO: check for lprint_sb->sb_data == NULL */
 			/* Adjust all values */
 			lprint_sb->sb_wptr = lprint_sb->sb_data + deltaw;
 			lprint_sb->sb_rptr = lprint_sb->sb_data + deltar;
@@ -715,6 +717,7 @@
 	
 	/* link it */
 	emup = (struct emu_t *)malloc(sizeof (struct emu_t));
+	/* TODO: check for emup == NULL */
 	emup->lport = (u_int16_t)lport;
 	emup->fport = (u_int16_t)fport;
 	emup->tos = tos;
@@ -889,10 +892,10 @@
 		/* Set the DISPLAY */
            if (x_port >= 0) {
 #ifdef HAVE_SETENV
-             sprintf(buff, "%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
+             snprintf(buff, sizeof(buff), "%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
              setenv("DISPLAY", buff, 1);
 #else
-             sprintf(buff, "DISPLAY=%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
+             snprintf(buff, sizeof(buff), "DISPLAY=%s:%d.%d", inet_ntoa(our_addr), x_port, x_screen);
              putenv(buff);
 #endif
            }
@@ -907,7 +910,7 @@
            
            /* Ooops, failed, let's tell the user why */
            
-           sprintf(buff, "Error: execlp of %s failed: %s\n", 
+           snprintf(buff, sizeof(buff), "Error: execlp of %s failed: %s\n", 
                    "rsh", strerror(errno));
            write(2, buff, strlen(buff)+1);
            close(0); close(1); close(2); /* XXX */
--- qemu-current/slirp/tcp_subr.c	2004-06-16 20:43:10.000000000 -0700
+++ qemu-dev/slirp/tcp_subr.c	2004-06-15 19:51:31.000000000 -0700
@@ -731,7 +731,7 @@
 				if (*ptr++ == 0) {
 					n++;
 					if (n == 2) {
-						sprintf(args, "rlogin -l %s %s",
+						snprintf(args, sizeof(args), "rlogin -l %s %s",
 							ptr, inet_ntoa(so->so_faddr));
 					} else if (n == 3) {
 						i2 = so_rcv->sb_wptr - ptr;
@@ -739,9 +739,9 @@
 							if (ptr[i] == '/') {
 								ptr[i] = 0;
 #ifdef HAVE_SETENV
-								sprintf(term, "%s", ptr);
+								snprintf(term, sizeof(term), "%s", ptr);
 #else
-								sprintf(term, "TERM=%s", ptr);
+								snprintf(term, sizeof(term), "TERM=%s", ptr);
 #endif
 								ptr[i] = '/';
 								break;
--- qemu-current/slirp/udp.c	2004-04-21 17:10:48.000000000 -0700
+++ qemu-dev/slirp/udp.c	2004-06-18 10:55:20.000000000 -0700
@@ -493,6 +493,7 @@
 			if (!req) {	/* no entry for so, create new */
 				req = (struct talk_request *)
 					malloc(sizeof(struct talk_request));
+				/* TODO: check for req == NULL */
 				req->udp_so = so;
 				req->tcp_so = solisten(0,		
 					OTOSIN(omsg, addr)->sin_addr.s_addr,	
--- qemu-current/target-i386/helper2.c	2004-05-08 14:05:19.000000000 -0700
+++ qemu-dev/target-i386/helper2.c	2004-06-18 08:56:24.000000000 -0700
@@ -212,7 +212,7 @@
     
     if (flags & X86_DUMP_CCOP) {
         if ((unsigned)env->cc_op < CC_OP_NB)
-            strcpy(cc_op_name, cc_op_str[env->cc_op]);
+            snprintf(cc_op_name, sizeof(cc_op_name), "%s", cc_op_str[env->cc_op]);
         else
             snprintf(cc_op_name, sizeof(cc_op_name), "[%d]", env->cc_op);
         fprintf(f, "CCS=%08x CCD=%08x CCO=%-8s\n",

Re: [Qemu-devel] Errors compiling QEMU with Mingw

[Vladimir N. Oleynik]

Filip,

 > Hi! I sent a patch for this few days ago. It's attached...

 > +#ifndef _WIN32
 > extern FILE *stdout;
 > +#else
 > +struct FILE { int opaque; };
 > +extern FILE (*_imp___iob)[];
 > +#define _iob	(*_imp___iob)
 > +#define stdout	(&_iob[1])
 > +#endif

But in fact this very dirty correction.
May be
#include <stdio.h>
is more true.


--w
vodz

Re: [Qemu-devel] [PATCH] security_20040618

[Vladimir N. Oleynik]

Tim,

 > Updates to this, from the previous version two days ago, include
 > additional strcpy replacements,

 > --- qemu-current/monitor.c	2004-06-16 20:49:59.000000000 -0700
 > +++ qemu-dev/monitor.c	2004-06-17 22:12:49.000000000 -0700
 >                  str = qemu_malloc(strlen(buf) + 1);
 > -                strcpy(str, buf);
 > +                pstrcpy(str, strlen(buf) + 1,  buf);

In my opinion, it already absolutely unnecessary correction.
There is in this place no problem.


--w
vodz

Re: [Qemu-devel] [PATCH] security_20040618

[Gianni Tedesco]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

On Fri, 2004-06-18 at 11:40 -0700, Tim wrote:
> Updates to this, from the previous version two days ago, include
> additional strcpy replacements, as well as TODO comments pointing out
> unhealthy calls to {m,re}alloc, which don't check for NULL return
> values. (I am not sure how to handle error return codes in most places,
> so just comments for now.)  Also pstrcpy() in vl.c was simplified, but
> should behave exactly the same as before.

In dyngen you need to do:

if ( ptr == NULL )
	error("malloc failed");

error() will never return.

For the other places it depends, but it's ususally quite simple. Why not
have a stab and submit a seperate patch on top of this one?

Also - Abother low hanging fruit may be /tmp file races. You could
probably make sure mkstmp is being used where possible etc.. and/or use
of /tmp files elimated as much as possible.... Or try setup a
$(HOME)/.qemu dir for that stuff. I know QEMU_TMPDIR is checked in vl.c
but the standard TMPDIR probably ought to be aswell if we DO use /tmp.

I mean, if root saves log to /tmp/qemu.log any user on the system may
obliterate any file (ln -s /etc/passwrd /tmp/qemu.log) as /tmp is the
default choice, perhaps root should know better, but maybe we should use
sane defaults like $(HOME)/qemu.log.

If people are interested in janitorial stuff like this, please, go right
ahead :)

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] security_20040618

[Tim]

> > --- qemu-current/monitor.c	2004-06-16 20:49:59.000000000 -0700
> > +++ qemu-dev/monitor.c	2004-06-17 22:12:49.000000000 -0700
> >                  str = qemu_malloc(strlen(buf) + 1);
> > -                strcpy(str, buf);
> > +                pstrcpy(str, strlen(buf) + 1,  buf);
> 
> In my opinion, it already absolutely unnecessary correction.
> There is in this place no problem.

Yeah, you are probably right.  I looked at that one on 3 seperate
occasions before making the change, since I recognized that there are
very few conditions where it could possibly be a problem, and come to
think of it, this fix doesn't mitigate those conditions.

That chunk of code makes me uncomfortable for other reasons though (does
qemu_malloc() return NULL ever?  could buf possibly be missing a
trailing '\0' ever?) so I'll re-visit it again and see what makes the
most sense.  The pstrcpy isn't hurting anything though.  Slightly slower
copy, due to the length checking, but it isn't in a critical piece of
code (monitor.c is just for the user interface command prompt, right?),
so I also don't see a reason to remove it, esp if changes in the future
open up the possibility of an overflow.

thanks for the comment,
tim

Re: [Qemu-devel] [PATCH] security_20040618

[Tim]

> In dyngen you need to do:
> 
> if ( ptr == NULL )
> 	error("malloc failed");
> 
> error() will never return.
>
> For the other places it depends, but it's ususally quite simple. Why not
> have a stab and submit a seperate patch on top of this one?

You would prefer I submit a seperate patch for the malloc fixes?  I
could certainly do that, but I just figured it is all a general code
quality patch, which changes very few lines of code (though in many
files).  What would you prefer Fabrice?
 
> Also - Abother low hanging fruit may be /tmp file races. You could
> probably make sure mkstmp is being used where possible etc.. and/or use
> of /tmp files elimated as much as possible.... Or try setup a
> $(HOME)/.qemu dir for that stuff. I know QEMU_TMPDIR is checked in vl.c
> but the standard TMPDIR probably ought to be aswell if we DO use /tmp.
> 
> I mean, if root saves log to /tmp/qemu.log any user on the system may
> obliterate any file (ln -s /etc/passwrd /tmp/qemu.log) as /tmp is the
> default choice, perhaps root should know better, but maybe we should use
> sane defaults like $(HOME)/qemu.log.

Heh, funny you mention it... that was next on my list.  I noticed the
logging to /tmp a while back, but didn't want to open up that can until
I understood the codebase a little better.

For my purposes, I would like to run qemu as more of a daemon than
anything.  So, I will likely attempt some improvements to the command
line logging options, and pick safe defaults.  I'll try to eliminate
other temp files where possible, and make the others secure against
races.  We'll see, I haven't even looked at that code yet... 

> If people are interested in janitorial stuff like this, please, go right
> ahead :)

Yeah, well I certainly am.  Call me anal retentive if you like, but I
really prefer to use software that's rock-solid when it comes to
stability and security.

tim

Re: [Qemu-devel] [PATCH] security_20040618

[Gianni Tedesco]

[-- Attachment #1: Type: text/plain, Size: 982 bytes --]

On Sat, 2004-06-19 at 08:19 -0700, Tim wrote:
> > In dyngen you need to do:
> > 
> > if ( ptr == NULL )
> > 	error("malloc failed");
> > 
> > error() will never return.
> >
> > For the other places it depends, but it's ususally quite simple. Why not
> > have a stab and submit a seperate patch on top of this one?
> 
> You would prefer I submit a seperate patch for the malloc fixes?  I
> could certainly do that, but I just figured it is all a general code
> quality patch, which changes very few lines of code (though in many
> files).  What would you prefer Fabrice?

Well, I meant if you were to go ahead and implement all the TODOs and
handle all the malloc returns properly. I'd submit that as a seperate
patch, i mean, if you aren't 100% confident of your analyses.

-- 
// Gianni Tedesco (gianni at scaramanga dot co dot uk)
lynx --source www.scaramanga.co.uk/scaramanga.asc | gpg --import
8646BE7D: 6D9F 2287 870E A2C9 8F60 3A3C 91B5 7669 8646 BE7D

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [Qemu-devel] [PATCH] security_20040618

[Fabrice Bellard]

Tim wrote:
> Updates to this, from the previous version two days ago, include
> additional strcpy replacements, as well as TODO comments pointing out
> unhealthy calls to {m,re}alloc, which don't check for NULL return
> values. (I am not sure how to handle error return codes in most places,
> so just comments for now.)  Also pstrcpy() in vl.c was simplified, but
> should behave exactly the same as before.

Thank you for the fixes.

For malloc() returning NULL, if your patch just says "malloc error", I 
don't consider it is a good fix.

I see that you added many snprintf(). I don't like that because 
pstrcpy() does exactly the right thing and it is faster and simpler.

Fabrice.

Re: [Qemu-devel] [PATCH] security_20040618

[Tim]

> Thank you for the fixes.

np.

> For malloc() returning NULL, if your patch just says "malloc error", I 
> don't consider it is a good fix.

Yeah, I figured it would be very context specific for these.  In
low-level functions, you'd just want to return an error value (that
would need to be checked as well) and at higher-level stuff, perhaps
you'd just bail out, and clean up before killing the process.

For now, I will leave my patch as it stands in this respect.  It doesn't
implement a single NULL check for {m,re}alloc, it merely adds TODO
comments.  Later on, after I address some other things, perhaps I will
come back and attack these with a different patch.

> I see that you added many snprintf(). I don't like that because 
> pstrcpy() does exactly the right thing and it is faster and simpler.

There were some cases where I switched sprintf() with snprintf().  For
those, I figure they should stay that way.  For the strcpy() ->
snprintf() changes, yes, I agree that your pstrcpy() is faster.  I'll
review those and change them where I can.  However, pstrcpy() may not be
available via includes to every file, so I'll probably let the
snprintf() stand if I run into that problem.

Were you able to look at my proposed changes to pstrcpy()?  What do you
think of them?

Thanks for the feedback!
tim

Re: [Qemu-devel] [PATCH] security_20040618

[Fabrice Bellard]

Tim wrote:

> Were you able to look at my proposed changes to pstrcpy()?  What do you
> think of them?

An advice: _never_ use strncpy, it never does what you think it does !

[Your patch is not correct as it adds a lot of zeros at the end of the 
buffer, which is not the intended behaviour of pstrcpy].

Fabrice.

[Qemu-devel] Re: [PATCH] security_20040618

[Charlie Gordon]

"Tim" <tim-qemu@sentinelchicken.org> wrote in message
news:20040619150514.GB1962@sentinelchicken.org...
> > > --- qemu-current/monitor.c 2004-06-16 20:49:59.000000000 -0700
> > > +++ qemu-dev/monitor.c 2004-06-17 22:12:49.000000000 -0700
> > >                  str = qemu_malloc(strlen(buf) + 1);
> > > -                strcpy(str, buf);
> > > +                pstrcpy(str, strlen(buf) + 1,  buf);
> >
> > In my opinion, it already absolutely unnecessary correction.
> > There is in this place no problem.
>
> Yeah, you are probably right.  I looked at that one on 3 seperate
> occasions before making the change, since I recognized that there are
> very few conditions where it could possibly be a problem, and come to
> think of it, this fix doesn't mitigate those conditions.
>
> That chunk of code makes me uncomfortable for other reasons though (does
> qemu_malloc() return NULL ever?  could buf possibly be missing a
> trailing '\0' ever?) so I'll re-visit it again and see what makes the
> most sense.  The pstrcpy isn't hurting anything though.  Slightly slower
> copy, due to the length checking, but it isn't in a critical piece of
> code (monitor.c is just for the user interface command prompt, right?),
> so I also don't see a reason to remove it, esp if changes in the future
> open up the possibility of an overflow.

No, he is ABSOLUTELY right !  This patch is useless and confusing.
pstrcpy's second argument is the size of the buffer the first argument
points to, computing it a second time is error prone as it duplicates
qemu_malloc size argument calculation.  Many modifications down the road,
these two lines may become sufficiently distant that a modification to one
will not be reflected in the other.  Furthermore, it is completely innane to
require protection from buffer overflow in pstrcpy by passing the exact size
of the third argument !  It reminds me of an overtly cautious programmer I
met a few years ago who made sure 'strings were properly null terminated by
adding : str[strlen(str)] = '\0';  what a joke!

Thus I see no reason to ACCEPT such a patch, removing it a just a matter of
cleanliness.

Still there was a more constructive remark to make on the above code as it
would definitely be better written as

str = qemu_strdup(buf);

with obvious semantics for qemu_strdup.

Anyone to write such a patch and use qemu_strdup in other places as
appropriate ?

Charlie Gordon.

PS: I fully agree with Fabrice about strncpy, disbelievers should read `man
strncpy` carefully and learn.

Re: [Qemu-devel] Re: [PATCH] security_20040618

[Tim]

> > Yeah, you are probably right.  I looked at that one on 3 seperate
> > occasions before making the change, since I recognized that there are
> > very few conditions where it could possibly be a problem, and come to
> > think of it, this fix doesn't mitigate those conditions.
> >
> > That chunk of code makes me uncomfortable for other reasons though (does
> > qemu_malloc() return NULL ever?  could buf possibly be missing a
> > trailing '\0' ever?) so I'll re-visit it again and see what makes the
> > most sense.  The pstrcpy isn't hurting anything though.  Slightly slower
> > copy, due to the length checking, but it isn't in a critical piece of
> > code (monitor.c is just for the user interface command prompt, right?),
> > so I also don't see a reason to remove it, esp if changes in the future
> > open up the possibility of an overflow.
> 
> No, he is ABSOLUTELY right !  This patch is useless and confusing.
> pstrcpy's second argument is the size of the buffer the first argument
> points to, computing it a second time is error prone as it duplicates
> qemu_malloc size argument calculation.  Many modifications down the road,
> these two lines may become sufficiently distant that a modification to one
> will not be reflected in the other.  Furthermore, it is completely innane to
> require protection from buffer overflow in pstrcpy by passing the exact size
> of the third argument !  It reminds me of an overtly cautious programmer I
> met a few years ago who made sure 'strings were properly null terminated by
> adding : str[strlen(str)] = '\0';  what a joke!
> 
> Thus I see no reason to ACCEPT such a patch, removing it a just a matter of
> cleanliness.
> 
> Still there was a more constructive remark to make on the above code as it
> would definitely be better written as
> 
> str = qemu_strdup(buf);
> 
> with obvious semantics for qemu_strdup.
> 
> Anyone to write such a patch and use qemu_strdup in other places as
> appropriate ?
> 
> Charlie Gordon.
> 
> PS: I fully agree with Fabrice about strncpy, disbelievers should read `man
> strncpy` carefully and learn.


Based on comments received thus far, including yours, I am reviewing
that section of code (as I mentioned above), and will be releasing a new
revision of the patch in a day or two.  I admit, I am not a perfect
programmer.  I am merely trying to help out by fixing the tiny problems
that are often missed by programmers that have more important things to
worry about.  I appreciate it when people show me where I am wrong, but
could you please keep your criticism a bit more constructive?

thanks,
tim

[Qemu-devel] Re: Re: [PATCH] security_20040618

[Charlie Gordon]

> Based on comments received thus far, including yours, I am reviewing
> that section of code (as I mentioned above), and will be releasing a new
> revision of the patch in a day or two.  I admit, I am not a perfect
> programmer.  I am merely trying to help out by fixing the tiny problems
> that are often missed by programmers that have more important things to
> worry about.  I appreciate it when people show me where I am wrong, but
> could you please keep your criticism a bit more constructive?

Sorry if I sounded a bit harsh, I'm sure every contribution is appreciated,
and your submitting patches is more helpful than my criticizing them...

I merely wanted to emphasize how broken strncpy is and how much more useful
pstrcpy is.

My suggestion on qemu_strdup is imho constructive :-)  and about the only
reason this is not completely off topic ;-)

But as far as strnpy is concerned, I *want* to be destructive : this C
library function is a mess, it doesn't do what most C programmers
believe.  It causes bugs, or blatant inefficiencies due to the inept null
padding on large buffers.
It is so unlikely that the precise behaviour of that horrible thing be what
is needed in any C program...
There are quite a few problems around uses of this function even in gnu
software or the linux kernel.

There are other candidates for libc functions every programmer should reject
disgruntedly : sprintf, gets, strtok, mktemp, tmpnam,
tempnam... or any of the C library functions duly tagged in the man pages as
never to be used
Similarly, extreme care is needed in do/while loops...

Charlie the C teaser.
---------------------
one of my favorite Q/As : what is wrong with : enum BOOL { FALSE=0,
TRUE=1 }; ?

Re: [Qemu-devel] Re: Re: [PATCH] security_20040618

[Tim]

> > Based on comments received thus far, including yours, I am reviewing
> > that section of code (as I mentioned above), and will be releasing a new
> > revision of the patch in a day or two.  I admit, I am not a perfect
> > programmer.  I am merely trying to help out by fixing the tiny problems
> > that are often missed by programmers that have more important things to
> > worry about.  I appreciate it when people show me where I am wrong, but
> > could you please keep your criticism a bit more constructive?
> 
> Sorry if I sounded a bit harsh, I'm sure every contribution is appreciated,
> and your submitting patches is more helpful than my criticizing them...

That's ok, I was just taken aback a bit.

> I merely wanted to emphasize how broken strncpy is and how much more useful
> pstrcpy is.
>
> My suggestion on qemu_strdup is imho constructive :-)  and about the only
> reason this is not completely off topic ;-)
> 
> But as far as strnpy is concerned, I *want* to be destructive : this C
> library function is a mess, it doesn't do what most C programmers
> believe.  It causes bugs, or blatant inefficiencies due to the inept null
> padding on large buffers.
> It is so unlikely that the precise behaviour of that horrible thing be what
> is needed in any C program...
> There are quite a few problems around uses of this function even in gnu
> software or the linux kernel.

Yes, since Fabrice pointed out the differences between my version of
pstrcpy() and his, I have come to appreciate pstrcpy()'s correctness and
speed.  I see what you mean by the problems with strncpy(), and I'll
make an effort to eliminate it with the same prejudice I have tried to
eliminate most strcpy() and sprintf() calls.

> There are other candidates for libc functions every programmer should reject
> disgruntedly : sprintf, gets, strtok, mktemp, tmpnam,
> tempnam... or any of the C library functions duly tagged in the man pages as
> never to be used
> Similarly, extreme care is needed in do/while loops...

Agreed.  In this day and age, there's really no excuse to use unsafe
functions such as these.  That's why I attempt to eliminate them with my
patch even though there may be no immediately obvious overflow issue.

Thanks for the additional comments & information.

cheers,
tim

OT: C Q/As, was Re: [Qemu-devel] security_20040618

[Christof Petig]

Charlie Gordon schrieb:
> Charlie the C teaser.
> ---------------------
> one of my favorite Q/As : what is wrong with : enum BOOL { FALSE=0,
> TRUE=1 }; ?

can you enlighten me? The only drawback I see is that with plain C (no 
C++) typedef enum { ... } BOOL; would be more appropriate.

I would propose
#ifndef __cplusplus
typedef enum { false, true } bool;
#endif
as the optimal solution for a problem I hardly have (since I usually 
don't go back to coding in C)

    Christof

PS: I used to ask: Why does this crash later (if you are lucky)

const char *itoa(int i)
{  char x[20];
    snprintf(x,sizeof x,"%d",i);
    return x;
}

[Qemu-devel] OT: C Q/As, was Re: security_20040618

[Charlie Gordon]

> > one of my favorite Q/As : what is wrong with : enum BOOL { FALSE=0,
> > TRUE=1 }; ?
>
> can you enlighten me? The only drawback I see is that with plain C (no
> C++) typedef enum { ... } BOOL; would be more appropriate.
>
defensive programming would require that TRUE be also defined as

#define TRUE 1

as many unsuspecting programmers will expect TRUE and FALSE to be handled in
the preprocessor phase eg:

#if TRUE
    ...
    somecode();
    ...
#endif

if TRUE is defined solely as en enum value, it will expand to 0 inside
preprocessing directive expressions without warning : quite a headache to
debug this type of mistake !


> PS: I used to ask: Why does this crash later (if you are lucky)
>
> const char *itoa(int i)
> {  char x[20];
>     snprintf(x,sizeof x,"%d",i);
>     return x;
> }

This code will abviously fail, unless you are very lucky and use the
returned value right away, as the x buffer is in automatic stack space and
will be overwritten by subsequent function calls or possibly signal
handlers.

I can think of 4 ways it may crash later when the pointer is dereferenced :
- you are using an advanced C compiler/runtime that checks pointer validity
(such as valgrind, checker, tinycc...)
- after the end of the thread that called itoa.
- if pages in the stack are unmapped for another reason like the stack being
swapped out and having shrunk below the depth it had when itoa was called
(quite unlikely).
- if the stack contains no zero byte till the end of its mapped space (you
would have to be so lucky for this!)

Anything more casual ?

Charlie, the C teaser.

Re: [Qemu-devel] OT: C Q/As, was Re: security_20040618

[Christof Petig]

Charlie Gordon schrieb:
>>>one of my favorite Q/As : what is wrong with : enum BOOL { FALSE=0,
>>>TRUE=1 }; ?
>>
>>can you enlighten me? The only drawback I see is that with plain C (no
>>C++) typedef enum { ... } BOOL; would be more appropriate.
>>
> 
> defensive programming would require that TRUE be also defined as
> 
> #define TRUE 1
> 
> as many unsuspecting programmers will expect TRUE and FALSE to be handled in
> the preprocessor phase eg:
> 
> #if TRUE
>     ...
>     somecode();
>     ...
> #endif
> 
> if TRUE is defined solely as en enum value, it will expand to 0 inside
> preprocessing directive expressions without warning : quite a headache to
> debug this type of mistake !

Thank you, I did not think of #if as a target for TRUE. That explains 
the numerous defines in gtk+ (instead of using enums).

Do you know what happens if you use '#define XYZ TRUE \n #if XYZ == 
TRUE' [If I read your answer correctly both == TRUE and == FALSE would 
be true :-( ]

>>PS: I used to ask: Why does this crash later (if you are lucky)
>>
>>const char *itoa(int i)
>>{  char x[20];
>>    snprintf(x,sizeof x,"%d",i);
>>    return x;
>>}

Oh I did not expect you to answer the obvious (to me). I just wanted to 
share my old would-be-employee test.

> This code will abviously fail, unless you are very lucky and use the
> returned value right away, as the x buffer is in automatic stack space and
> will be overwritten by subsequent function calls or possibly signal
> handlers.
> 
> I can think of 4 ways it may crash later when the pointer is dereferenced :
> - you are using an advanced C compiler/runtime that checks pointer validity
> (such as valgrind, checker, tinycc...)

can valgrind really check it if you dereference the pointer in a 
subroutine deeply enough to reuse the memory again?

> - after the end of the thread that called itoa.
> - if pages in the stack are unmapped for another reason like the stack being
> swapped out and having shrunk below the depth it had when itoa was called
> (quite unlikely).
> - if the stack contains no zero byte till the end of its mapped space (you
> would have to be so lucky for this!)
> 
> Anything more casual ?

that's more than I ever expected ;-) You are right, crashing is far more 
unlikely than giving strange characters. If you rely on the length or 
content of the string returned, that might easily lead to a crash (if 
you make other mistakes).

actually knowing that a static will fix the bug (but introduce new 
problems) should be enough for most C programmers.

     Christof

Re: OT: C Q/As, was Re: [Qemu-devel] security_20040618

[Michael Jennings]

On Monday, 21 June 2004, at 10:50:44 (+0200),
Christof Petig wrote:

> can you enlighten me? The only drawback I see is that with plain C
> (no C++) typedef enum { ... } BOOL; would be more appropriate.
> 
> I would propose
> #ifndef __cplusplus
> typedef enum { false, true } bool;
> #endif
> as the optimal solution for a problem I hardly have (since I usually 
> don't go back to coding in C)

There are two problems with using enum's and/or #define's for
TRUE/FALSE.  They should not be used in boolean expressions except
where the return value is of that same typedef (e.g., the function
returns bool) or is generated from the same set of #define's.  While
false is always 0, true is not always 1.  True is non-zero.

> const char *itoa(int i)
> {  char x[20];
>    snprintf(x,sizeof x,"%d",i);
>    return x;
> }

Forgot "static" before char x[20];, or to be more threadsafe, either
malloc() or pass in a buffer and max size.

> defensive programming would require that TRUE be also defined as
> 
> #define TRUE 1
> 
> as many unsuspecting programmers will expect TRUE and FALSE to be handled in
> the preprocessor phase eg:
> 
> #if TRUE
>     ...
>     somecode();
>     ...
> #endif

I disagree strongly.  Anyone who writes "#if TRUE" or "#if FALSE" is
just asking for trouble; (s)he needs to be taught a lesson.  "#if 0"
and "#if 1" are just as obvious, and both "#if 1" and "#if TRUE" are
equally ridiculous.

A better technique would be something more like this:

#define UNUSED_CODE_BLOCK 0

#if UNUSED_CODE_BLOCK
...
#else  /* used code below */
...
#endif

But even then it isn't as clean and readable as 1 or 0.  Plus, you've
still failed to solve the problem that true may or may not be 1.

Michael

-- 
Michael Jennings (a.k.a. KainX)  http://www.kainx.org/  <mej@kainx.org>
n + 1, Inc., http://www.nplus1.net/       Author, Eterm (www.eterm.org)
-----------------------------------------------------------------------
 "You can accomplish much if you don't care who gets the credit."
                                                      -- Ronald Reagan

[Qemu-devel] Re: completely OT: C Q/As, was Re: security_20040618

[Charlie Gordon]

"Michael Jennings" <mej@eterm.org> wrote in message
news:20040621154440.GQ4686@kainx.org...
> There are two problems with using enum's and/or #define's for
> TRUE/FALSE.  They should not be used in boolean expressions except
> where the return value is of that same typedef (e.g., the function
> returns bool) or is generated from the same set of #define's.  While
> false is always 0, true is not always 1.  True is non-zero.

This is a common misunderstanding.  your last statement doesn't make sense :
in C, false IS 0 and true IS 1.  That is the result of a false comparison is
0 (eg: 2 == 3) and the result of a true comparison is 1 (as in 2 == 2).  As
such boolean expressions can only have 2 values : 0 and 1, appropriately
defined as FALSE and TRUE, be it via #define or enum.
Unlike its distant cousin Java, C allows non boolean expressions to control
test statements such as 'if', 'for', and 'while'.  In such constructs,
falseness is not limited to the integer value 0 : '\0', 0.0, and NULL
pointers also convert to false, while all other values convert to true.
Further confusion is caused by the convention used in the standard C library
where boolean valued function do not necessarily return 1 for trueness.
Furthermore, it is perfectly OK to compare values from different typedefs
but identical implementation types.
I do agree with you that there is room for confusion, especially for java
bred programmers, and it is usually better to avoid the extra clutter of
comparing to FALSE or TRUE in test expressions : just use the plain
expression or bang (!) it for the reverse condition.

> Forgot "static" before char x[20];, or to be more threadsafe, either
> malloc() or pass in a buffer and max size.

These are obvious solutions to the problem, the latter (buffer and size) is
actually the one used in the C library for that very function (itoa).  But
the question was a different one : Why does this crash later (if you are
lucky) ?

> > defensive programming would require that TRUE be also defined as
> >
> > #define TRUE 1
> >
> > as many unsuspecting programmers will expect TRUE and FALSE to be
handled in
> > the preprocessor phase eg:
> >
> > #if TRUE
> >     ...
> >     somecode();
> >     ...
> > #endif
>
> I disagree strongly.  Anyone who writes "#if TRUE" or "#if FALSE" is
> just asking for trouble; (s)he needs to be taught a lesson.  "#if 0"
> and "#if 1" are just as obvious, and both "#if 1" and "#if TRUE" are
> equally ridiculous.

You use #if 0 yourself, try pretending you never switch those to #if 1 ?
I used to be harsh like this and show contempt for imperfect programming.
I've had to grow more tolerant, as 99% of C or C++ code is indeed imperfect,
and most programmers are more focussed on problem solving than clean coding.
The truth is that *anyone* who writes C code is just asking for trouble.
Yet this is still my favorite language.  I do use #if 0 or #if 1 sometimes,
never #if TRUE or #if FALSE, but I do not believe people that do should be
taught such a difficult lesson : after all, it is an accepted convention
that all uppercase identifiers be preprocessor defined. Christof Petig
provided a more compelling example :

#define USE_MORE_CAUTION  TRUE
#if USE_MORE_CAUTION == TRUE
    // this code would compile
#endif
#if USE_MORE_CAUTION == FALSE
    // this code would compile as well
#endif

My point is that if you define TRUE and FALSE in a header file, your job is
to make sure that you are not setting up boobytraps for the unsuspecting
(and imperfect) programmer to trigger.

I took a look at libast and you DO redefine TRUE and FALSE so as to setup
such a trap !!! have pity on your users!

/* from libast/types.h */
...
#undef false
#undef False
#undef FALSE
#undef true
#undef True
#undef TRUE

typedef enum {
#ifndef __cplusplus
  false = 0,
#endif
  False = 0,
  FALSE = 0,
#ifndef __cplusplus
  true = 1,
#endif
  True = 1,
  TRUE = 1
} spif_bool_t;
...

While your code is amazingly consistent and readable (except for line
length), you can learn from this very thread and fix it : you use strncpy 10
times. 7 of those are incorrect and at least 5 will lead to potential buffer
overflow and inefficiencies, the remaining 3 could be replaced directly with
calls to memcpy.  You make one incorrect use of strncat, that may overflow
by just one byte.  pstrcpy provides the very semantics you require in most
of these cases.

Cheers,

Chqrlie.

Re: [Qemu-devel] Re: completely OT: C Q/As, was Re: security_20040618

[Sander Nagtegaal]

Something else...............I'm new to C++ ( but this is C code but the same 
in C++) so don't shoot me.......
Isn't enum { FALSE=0 , TRUE=1 } compleetly stupid anyway. I mean........is 
FALSE=0 then enum will automaticly make TRUE 1 right? So then 
enum {FALSE=0 , TRUE } ; would be better right?
Again don't shoot me I'm just trying to learn this language......

Op dinsdag 22 juni 2004 11:57 am, schreef Charlie Gordon:
> "Michael Jennings" <mej@eterm.org> wrote in message
> news:20040621154440.GQ4686@kainx.org...
>
> > There are two problems with using enum's and/or #define's for
> > TRUE/FALSE.  They should not be used in boolean expressions except
> > where the return value is of that same typedef (e.g., the function
> > returns bool) or is generated from the same set of #define's.  While
> > false is always 0, true is not always 1.  True is non-zero.
>
> This is a common misunderstanding.  your last statement doesn't make sense
> : in C, false IS 0 and true IS 1.  That is the result of a false comparison
> is 0 (eg: 2 == 3) and the result of a true comparison is 1 (as in 2 == 2). 
> As such boolean expressions can only have 2 values : 0 and 1, appropriately
> defined as FALSE and TRUE, be it via #define or enum.
> Unlike its distant cousin Java, C allows non boolean expressions to control
> test statements such as 'if', 'for', and 'while'.  In such constructs,
> falseness is not limited to the integer value 0 : '\0', 0.0, and NULL
> pointers also convert to false, while all other values convert to true.
> Further confusion is caused by the convention used in the standard C
> library where boolean valued function do not necessarily return 1 for
> trueness. Furthermore, it is perfectly OK to compare values from different
> typedefs but identical implementation types.
> I do agree with you that there is room for confusion, especially for java
> bred programmers, and it is usually better to avoid the extra clutter of
> comparing to FALSE or TRUE in test expressions : just use the plain
> expression or bang (!) it for the reverse condition.
>
> > Forgot "static" before char x[20];, or to be more threadsafe, either
> > malloc() or pass in a buffer and max size.
>
> These are obvious solutions to the problem, the latter (buffer and size) is
> actually the one used in the C library for that very function (itoa).  But
> the question was a different one : Why does this crash later (if you are
> lucky) ?
>
> > > defensive programming would require that TRUE be also defined as
> > >
> > > #define TRUE 1
> > >
> > > as many unsuspecting programmers will expect TRUE and FALSE to be
>
> handled in
>
> > > the preprocessor phase eg:
> > >
> > > #if TRUE
> > >     ...
> > >     somecode();
> > >     ...
> > > #endif
> >
> > I disagree strongly.  Anyone who writes "#if TRUE" or "#if FALSE" is
> > just asking for trouble; (s)he needs to be taught a lesson.  "#if 0"
> > and "#if 1" are just as obvious, and both "#if 1" and "#if TRUE" are
> > equally ridiculous.
>
> You use #if 0 yourself, try pretending you never switch those to #if 1 ?
> I used to be harsh like this and show contempt for imperfect programming.
> I've had to grow more tolerant, as 99% of C or C++ code is indeed
> imperfect, and most programmers are more focussed on problem solving than
> clean coding. The truth is that *anyone* who writes C code is just asking
> for trouble. Yet this is still my favorite language.  I do use #if 0 or #if
> 1 sometimes, never #if TRUE or #if FALSE, but I do not believe people that
> do should be taught such a difficult lesson : after all, it is an accepted
> convention that all uppercase identifiers be preprocessor defined. Christof
> Petig provided a more compelling example :
>
> #define USE_MORE_CAUTION  TRUE
> #if USE_MORE_CAUTION == TRUE
>     // this code would compile
> #endif
> #if USE_MORE_CAUTION == FALSE
>     // this code would compile as well
> #endif
>
> My point is that if you define TRUE and FALSE in a header file, your job is
> to make sure that you are not setting up boobytraps for the unsuspecting
> (and imperfect) programmer to trigger.
>
> I took a look at libast and you DO redefine TRUE and FALSE so as to setup
> such a trap !!! have pity on your users!
>
> /* from libast/types.h */
> ...
> #undef false
> #undef False
> #undef FALSE
> #undef true
> #undef True
> #undef TRUE
>
> typedef enum {
> #ifndef __cplusplus
>   false = 0,
> #endif
>   False = 0,
>   FALSE = 0,
> #ifndef __cplusplus
>   true = 1,
> #endif
>   True = 1,
>   TRUE = 1
> } spif_bool_t;
> ...
>
> While your code is amazingly consistent and readable (except for line
> length), you can learn from this very thread and fix it : you use strncpy
> 10 times. 7 of those are incorrect and at least 5 will lead to potential
> buffer overflow and inefficiencies, the remaining 3 could be replaced
> directly with calls to memcpy.  You make one incorrect use of strncat, that
> may overflow by just one byte.  pstrcpy provides the very semantics you
> require in most of these cases.
>
> Cheers,
>
> Chqrlie.
>
>
>
>
>
>
>
>
> _______________________________________________
> Qemu-devel mailing list
> Qemu-devel@nongnu.org
> http://lists.nongnu.org/mailman/listinfo/qemu-devel

[Qemu-devel] Re: Re: completely OT: C Q/As, was Re: security_20040618

[Charlie Gordon]

"Sander Nagtegaal" <trunks-carracho@planet.nl> wrote in message
news:200406221249.36994.trunks-carracho@planet.nl...
> Something else...............I'm new to C++ ( but this is C code but the
same
> in C++) so don't shoot me.......

I'm just a teaser, not an executionner.

> Isn't enum { FALSE=0 , TRUE=1 } compleetly stupid anyway. I mean........is
> FALSE=0 then enum will automaticly make TRUE 1 right? So then
> enum {FALSE=0 , TRUE } ; would be better right?

Well, yes and no.  You correctly analyse that the initializer for TRUE is
redundant, as a matter of fact both initialisers are useless.
So I could have written :

enum BOOL { FALSE, TRUE };
or whatever typedef is appropriate.

But I prefer to make certain things more explicit.  I find it more readable.
Not exactly what you call "completely stupid" !

Now if you want to flame something completely stupid, look at this one :

#define FALSE 0==1
#define TRUE  1==1

Why will this fail ?

Chqrlie.

Re: [Qemu-devel] Re: completely OT: C Q/As

[Michael Jennings]

On Tuesday, 22 June 2004, at 11:57:23 (+0200),
Charlie Gordon wrote:

> This is a common misunderstanding.  your last statement doesn't make
> sense : in C, false IS 0 and true IS 1.  That is the result of a
> false comparison is 0 (eg: 2 == 3) and the result of a true
> comparison is 1 (as in 2 == 2).  As such boolean expressions can
> only have 2 values : 0 and 1, appropriately defined as FALSE and
> TRUE, be it via #define or enum.

Incorrect.  The result of a boolean operator is guaranteed to be 1 or
0.  However, values evaluated in a boolean context such as those in an
if statement are not bound by that rule.

> Unlike its distant cousin Java, C allows non boolean expressions to
> control test statements such as 'if', 'for', and 'while'.  In such
> constructs, falseness is not limited to the integer value 0 : '\0',
> 0.0, and NULL pointers also convert to false, while all other values
> convert to true.

All the values you mentioned evaluate to (int) 0 in a boolean context.

> Further confusion is caused by the convention used in the standard C
> library where boolean valued function do not necessarily return 1
> for trueness.

As there is no such thing as a "boolean value" in C, no standard C
function returns a boolean value.  Some do, however, return an integer
that may be successfully used in a boolean context (albeit possibly
negated).

> I do agree with you that there is room for confusion, especially for
> java bred programmers, and it is usually better to avoid the extra
> clutter of comparing to FALSE or TRUE in test expressions : just use
> the plain expression or bang (!) it for the reverse condition.

That was not my statement.  Again, it depends on what you're doing.
If you're guaranteed a return of 0 or non-zero, best to use == 0 or !=
0.  With pointers, the value itself is fine, negated or not, but other
valid approaches include == NULL, != NULL, or even a macro like
ISNULL().

> These are obvious solutions to the problem, the latter (buffer and
> size) is actually the one used in the C library for that very
> function (itoa).  But the question was a different one : Why does
> this crash later (if you are lucky) ?

You found my answer to be obvious.  I found the question to be
obvious.  So we're even. :-)

The answer you were looking for (regarding the stack) had already been
given.  I was simply taking a different tack.

> You use #if 0 yourself, try pretending you never switch those to #if 1 ?

In production-quality code?  Never.  For testing purposes where only I
was meant to see it?  Absolutely.

> I used to be harsh like this and show contempt for imperfect
> programming.  I've had to grow more tolerant, as 99% of C or C++
> code is indeed imperfect, and most programmers are more focussed on
> problem solving than clean coding.

And a lackadaisical attitude such as this is why so many programmers
just don't know any better.  Be tolerant in what you accept, but
strict in what you create.  And just because something doesn't
generate an error doesn't mean it shouldn't generate a warning.

> The truth is that *anyone* who writes C code is just asking for
> trouble.

Don't agree there.

> after all, it is an accepted convention that all uppercase
> identifiers be preprocessor defined.

We agree on that point at least. :-)

> Christof Petig provided a more compelling example :
> 
> #define USE_MORE_CAUTION  TRUE
> #if USE_MORE_CAUTION == TRUE
>     // this code would compile
> #endif
> #if USE_MORE_CAUTION == FALSE
>     // this code would compile as well
> #endif

Again, it's more properly written as "#if USE_MORE_CAUTION" or "#if
!USE_MORE_CAUTION"

> My point is that if you define TRUE and FALSE in a header file, your
> job is to make sure that you are not setting up boobytraps for the
> unsuspecting (and imperfect) programmer to trigger.

And again, I disagree.  By defining something in a header file that
other programmers will use, you're defining an API.  What is most
important is that it is clear and consistent, not that it follows any
one particular religion.

> I took a look at libast and you DO redefine TRUE and FALSE so as to
> setup such a trap !!! have pity on your users!
> 
> /* from libast/types.h */
> ...
> #undef false
> #undef False
> #undef FALSE
> #undef true
> #undef True
> #undef TRUE
> 
> typedef enum {
> #ifndef __cplusplus
>   false = 0,
> #endif
>   False = 0,
>   FALSE = 0,
> #ifndef __cplusplus
>   true = 1,
> #endif
>   True = 1,
>   TRUE = 1
> } spif_bool_t;
> ...

First off, I'm creating a well-defined API.  I use an enum to create
an actual type I can return, manipulate, check against, blah blah
blah.  I undefine those macros to prevent situations where some
platforms may expand one or more of the LHS values to something that's
no longer valid for the enum while others may not.

Secondly, libast headers are API headers, and are really not all that
different from system headers.  If a programmer wishes to use them,
(s)he should #include them BEFORE any custom headers (s)he may also be
using.  Nothing is stopping a programmer from redefining those macros
as (s)he sees fit.  But for my purposes, the correct choice is clear.

Besides, several other items which are more intended for preprocessor
use (e.g., FIXME_BLOCK and UNUSED_BLOCK) are defined separately.

> While your code is amazingly consistent and readable (except for
> line length),

LOL  You've got me there; I use GNU Emacs under X11, so my maximum
line length is largely dependent on how big the Emacs window was in
which I wrote that code. :-)

Wrapping code at 80 chars can result in some really ugly messes, so I
gave up on that.  I just haven't found a really good alternative yet.

> you can learn from this very thread and fix it : you use strncpy 10
> times. 7 of those are incorrect and at least 5 will lead to
> potential buffer overflow and inefficiencies,

My analysis of the code disagrees, but I'd like to look at any
evidence you may have.

> the remaining 3 could be replaced directly with calls to memcpy.
> You make one incorrect use of strncat, that may overflow by just one
> byte.

You're right, thanks.

Michael

-- 
Michael Jennings (a.k.a. KainX)  http://www.kainx.org/  <mej@kainx.org>
n + 1, Inc., http://www.nplus1.net/       Author, Eterm (www.eterm.org)
-----------------------------------------------------------------------
 "Did you really have to die for me?  All I am for all You are because
  What I need and what I believe are worlds apart."
                                       -- Jars of Clay, "Worlds Apart"

[Qemu-devel] Re: Re: completely OT: C Q/As : let's feed the troll

[Charlie Gordon]

> As there is no such thing as a "boolean value" in C, no standard C
> function returns a boolean value.  Some do, however, return an integer
> that may be successfully used in a boolean context (albeit possibly
> negated).

In C there is no boolean type, but there are boolean values : results of
boolean operators as you pointed out.
I agree that most standard C functions do not return booleans values, which
is the cause of numerous bugs by unsuspecting but bold programmers, not
necessarily newbies. look at these examples:

if (isspace(*p) == isspace(*q)) ...   // stupid optimisation!
if (isdir(path1) & isdir(path2)) ...     // lack of C experience or plain
typo

These are horrible things to write, but I've seen much worse examples of non
portable code like this, that do work in a lot of environments, and will
fail without warning on others.
One would need to define an integer type that can only be compared to 0.
strcmp provides even more horrible semantics: C newbies routinely compare
the result to -1 !
Do you know any tool that can detect such horrors ?  I agree with teaching
offenders a lesson, that's what compile time warnings and errors are for.
If you let bugs like this crawl into production software, end users will
become victims, and maintainers will do the hard work...  Original
authors/sinners will have already moved on to other projects, and commit
more trash.

 My point is this : C is difficult enough as a language, try not to add more
traps unnecessarily.

> And just because something doesn't
> generate an error doesn't mean it shouldn't generate a warning.
>

I agree completely, as a matter of fact, I always compile with almost all
warnings on, and make gcc treat them as errors.

> > /* from libast/types.h */
> > ...
> > #undef false
> > #undef False
> > #undef FALSE
> > #undef true
> > #undef True
> > #undef TRUE
> >
> > typedef enum {
> > #ifndef __cplusplus
> >   false = 0,
> > #endif
> >   False = 0,
> >   FALSE = 0,
> > #ifndef __cplusplus
> >   true = 1,
> > #endif
> >   True = 1,
> >   TRUE = 1
> > } spif_bool_t;
> > ...
>
> First off, I'm creating a well-defined API.  I use an enum to create
> an actual type I can return, manipulate, check against, blah blah
> blah.  I undefine those macros to prevent situations where some
> platforms may expand one or more of the LHS values to something that's
> no longer valid for the enum while others may not.
>
> Secondly, libast headers are API headers, and are really not all that
> different from system headers.  If a programmer wishes to use them,
> (s)he should #include them BEFORE any custom headers (s)he may also be
> using.  Nothing is stopping a programmer from redefining those macros
> as (s)he sees fit.  But for my purposes, the correct choice is clear.

In the case of FALSE/TRUE, you know damn well you are not "creating" the
API, you are potentially redefining it, the fact that you first undef those
symbols proves my point.  I cannot understand why you define upto 3
different symbols for each boolean value !
In my original post, I was pointing out how such a redefinition has subtle
side-effects that you probably didn't anticipate.

For completeness, TRUE and FALSE are defined by numerous API headers.
Apache for one has a bogus definition in several header files :
apache/ap_ctx.h:
#ifndef FALSE
#define FALSE 0
#define TRUE  !FALSE
#endif

The missing parentheses should strike as an obvious mistake.
Further thinking may convince you otherwise...
Wrong ! here is a contrived counterexample :

    printf("this should print 98 : TRUE[\"ab\"]=%d\n", TRUE["ab"]);

will print 0 instead of 98 with that definition.

C IS DIFFICULT ! There is always one more bug.

> > While your code is amazingly consistent and readable (except for
> > line length),
>
> LOL  You've got me there; I use GNU Emacs under X11, so my maximum
> line length is largely dependent on how big the Emacs window was in
> which I wrote that code. :-)
>
> Wrapping code at 80 chars can result in some really ugly messes, so I
> gave up on that.  I just haven't found a really good alternative yet.

Good for you !  I can read C code much faster when it doesn't extend beyond
80 columns.
You get ugly messes because you do too much on one line, use too many
verbose macros...

    self->items = SPIF_CAST_C(spif_obj_t *) REALLOC(self->items,
SPIF_SIZEOF_TYPE(obj) * (--(self->len)));

Since REALLOC, and MALLOC always need a cast in C++, why not pass the type ?
Since realloc has despicable behaviour when running out of memory, why not
pass the address of the pointer ?
Then wrap the ugly mess in a macro (you know about that)...
This is much safer, more concise, and fits in 80 columns, and you could even
check the boolean return value :

    REALLOC(&self->items, --self->len, spif_obj_t);

> > you can learn from this very thread and fix it : you use strncpy 10
> > times. 7 of those are incorrect and at least 5 will lead to
> > potential buffer overflow and inefficiencies,
>
> My analysis of the code disagrees, but I'd like to look at any
> evidence you may have.

Lets go:

socket.c line 717
    addr->sun_path[0] = 0;
    strncat(addr->sun_path, SPIF_STR_STR(spif_url_get_path(self)),
sizeof(addr->sun_path));

strncat will copy upto sizeof(addr->sun_path) bytes from the source, and
tack a '\0'.
the destination happens to be empty (why use strncat ?), so this code can
only overflow addr->sun_path by 1 byte, it would be much worse if actual
concatenation took place.

file.c line 51:
    if (len) {
        strncpy(ftemplate, buff, len);
        ftemplate[len - 1] = 0;
    }

this is just inefficient and cumbersome. pstrcpy is what you want.  also len
is a misleading name for the buffer size.

mem.c line 78:

    strncpy(p->file, filename, LIBAST_FNAME_LEN);
    p->file[LIBAST_FNAME_LEN] = 0;

inefficiency again, luckily the buffer size is LIBAST_FNAME_LEN + 1, pstrcpy
fixes this.

mem.c line 135

   strncpy(p->file, filename, LIBAST_FNAME_LEN);

no extra null setting here ! code is inconsistent, I guess you rely on magic
side effects : the ptr_t structure is not allocated here and most likely the
last byte of the file buffer has already been set to 0.  This is unsafe.
pstrcpy fixes this.

strings.c line 99. line 120:

    tmpstr = (char *) MALLOC(cnt + 1);
    strncpy(tmpstr, str, cnt);
    tmpstr[cnt] = 0;
    return (tmpstr);

inefficiency !  pstrcpy fixes this.

conf.c line 485:

                  strncpy(newbuff + j, getenv("HOME"), max - j);
                  cnt1 = strlen(getenv("HOME")) - 1;
                  cnt2 = max - j - 1;
                  j += MIN(cnt1, cnt2);

given CONFIG_BUFF is defined as 20480, (max - j) is likely very large. So
inefficient!
It is also bogus as newbuff is not guarantied to be null terminated.
getting $HOME twice is childish.
relying on MIN to fix the unsigned overflow in case $HOME is the empty
string is nothing to be proud of.
Similar code is repeated 3 more times in the same function with similar
problems.

conf.c line 741:
        if (n > 0 && n <= maxpathlen) {
            /* Compose the /path/file combo */
            strncpy(full_path, path, n);
            if (full_path[n - 1] != '/') {
                full_path[n++] = '/';
            }
            full_path[n] = '\0';
            strcat(full_path, name);

strncpy is OK here, but not needed as memcpy can be used directly.
strcpy(full_path + n, name) would be more efficient as well.

I'll grant you that the bugs are very remote, but that makes them completely
unlikely to be caught
before production.

Just don't use strncpy, steal pstrcpy from here and use it, and propagate to
message !


Cheers,

Charlie Gordon.


PS: please let's stop polluting qemu, address further remarks to me
directly.