Sun, 06 May 2012

Open sores

After complaining about Microsoft last time I figured I'd do something different this time: I'm going to complain about a piece of open source software.

It needs no introduction, but I'll give it one anyway: The ISC DHCP server and client are the standard DHCP(v4/v6) implementations and they're used all over the place.
Recently I was fixing a bug in a dhclient-script.sh. It incorrectly parsed an IAID value because it contained an '='.

The relevant bits of source code: 

	ient_envadd(client, prefix, "iaid", "%s",
			print_hex_1(4, ia->iaid, 12));
This just adds the IAID value to the environment encoded, you'd expect, as a hex string.
Hang on? Hex string? Didn't I just say that we got an '=' in the data?

Looking a little deeper there's the first disturbing bit: 

#define print_hex_1(len, data, limit) print_hex(len, data, limit, 0)
#define print_hex_2(len, data, limit) print_hex(len, data, limit, 1)
#define print_hex_3(len, data, limit) print_hex(len, data, limit, 2)
Umm, ok then. 

#define HBLEN 1024
char *print_hex(len, data, limit, buf_num)
        unsigned len;       
        const u_int8_t *data;
        unsigned limit;      
        unsigned buf_num;    
{
        static char hex_buf_1[HBLEN + 1];
        static char hex_buf_2[HBLEN + 1];
        static char hex_buf_3[HBLEN + 1];
        char *hex_buf;

        switch(buf_num) {
          case 0:
                hex_buf = hex_buf_1;
                if (limit >= sizeof(hex_buf_1))
                        limit = sizeof(hex_buf_1);
                break;
          case 1:
                hex_buf = hex_buf_2;
                if (limit >= sizeof(hex_buf_2)) 
                        limit = sizeof(hex_buf_2);
                break;   
          case 2:        
                hex_buf = hex_buf_3;
                if (limit >= sizeof(hex_buf_3))
                        limit = sizeof(hex_buf_3);
                break;
          default:
                return(NULL);
        }

        print_hex_or_string(len, data, limit, hex_buf);
        return(hex_buf);
}
Wait what? What's with the three static buffers?
It's an evil, and stupid little trick to avoid having to supply a buffer from the caller. That's why there's a static buffer: the caller can just use the returned pointer without having to worry about freeing allocated memory.
There's three of them because presumably at some point someone tried to convert two strings before printing them and discovered that only both always had the same content when he used the output. Instead of solving the problem properly he decided to use this disgusting hack instead.
That's bad, but what about print_hex_or_string? 

/*      
 * print a string as either text if all the characters
 * are printable or colon separated hex if they aren't
 *        
 * len - length of data 
 * data - input data
 * limit - length of buf to use 
 * buf - output buffer
 */       
void print_hex_or_string (len, data, limit, buf)
        unsigned len;
        const u_int8_t *data; 
        unsigned limit;
        char *buf;
{               
        unsigned i;
        if ((buf == NULL) || (limit < 3))
                return;
          
        for (i = 0; (i < (limit - 3)) && (i < len); i++) {
                if (!isascii(data[i]) || !isprint(data[i])) {
                        print_hex_only(len, data, limit, buf);
                        return;
                }
        }

        buf[0] = '"';
        i = len;
        if (i > (limit - 3))
                i = limit - 3;
        memcpy(&buf[1], data, i);
        buf[i + 1] = '"';
        buf[i + 2] = 0;
        return;
}
Well, that's about as bad as the function name sounded. This converts the supplied data into a string, either by interpreting it as plain ASCII (if all of the bytes are printable), or converting it into a real hex string.
Enjoy yourself parsing that. Writing parsing and validation code is so much fun and now you get to do it twice!

posted at: 23:22 | path: / | [ 1 comments ]