[PATCH] v3: Improve printk reliability

[Carl-Daniel Hailfinger]

This patch tries to make printk more readable and reliable.

Base 16 number printing used digits[33] instead of the more readable 'x'
for the "0x" prefix. That means you have to look up an array just to
find out what the code does. Change it.

We already write "<NULL>" if printk gets a NULL pointer as string
argument. Introduce "<almost NULL>" for string arguments with addresses
below 0x400. This error happened in the past when dereferencing a struct
member with a string and the struct had the address NULL.

Check if all to-be-printed characters in the string are indeed
printable. The idea is to catch garbage strings from stray pointers and
print "<GARBAGE>" instead. This is the only part of the patch I'm not so
sure about. If a string contains characters outside classic ASCII
printable stuff, this will trigger. An example would be characters with
diacritic marks like äöüéăçőč. Then again, I don't see localized
versions of coreboot on the horizon. Maybe for payloads, but not for
coreboot itself.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-printk_safe_strings/lib/vtxprintf.c
===================================================================
--- corebootv3-printk_safe_strings/lib/vtxprintf.c	(Revision 1157)
+++ corebootv3-printk_safe_strings/lib/vtxprintf.c	(Arbeitskopie)
@@ -16,6 +16,7 @@
 #define isdigit(c)	((c) >= '0' && (c) <= '9')
 #define is_digit isdigit
 #define isxdigit(c)	(((c) >= '0' && (c) <= '9') || ((c) >= 'a' && (c) <= 'f') || ((c) >= 'A' && (c) <= 'F'))
+#define isprint(c)	((c) == '\n' || ((c) >= ' ' && (c) <= '~'))

static int skip_atoi(const char **s)
 {
@@ -87,7 +88,7 @@
    		tx_byte('0', arg), count++;
    	else if (base==16) {
    		tx_byte('0', arg), count++;
-			tx_byte(digits[33], arg), count++;
+			tx_byte('x', arg), count++;
    	}
    }
    if (!(type & LEFT))
@@ -194,9 +195,15 @@
    		s = va_arg(args, char *);
    		if (!s)
    			s = "<NULL>";
+			/* Catch almost-NULL pointers as well */
+			if ((size_t)s < 0x400)
+				s = "<almost NULL>";

len = strnlen(s, precision);

+			for (i = 0; i < len; ++i)
+				if (!isprint(*s[i]))
+					s = "<GARBAGE>";
    		if (!(flags & LEFT))
    			while (len < field_width--)
    				tx_byte(' ', arg), count++;

[Peter Stuge]

Carl-Daniel Hailfinger wrote:
> This patch tries to make printk more readable and reliable.

I like the idea.
> Introduce "<almost NULL>" for string arguments with addresses
> below 0x400.

I would like the message to be more informative. Include the address
somehow..

"<near-NULL %03x>" or so?

(No, I don't want to make printf() recurse, but you get my idea. :)
> The idea is to catch garbage strings from stray pointers and print
> "<GARBAGE>" instead.

Again request more informative text. For example I think
"<non-ASCII characters>" would be much nicer.
> Signed-off-by: Carl-Daniel Hailfinger

If you agree with my requests I say

Acked-by: Peter Stuge

[Carl-Daniel Hailfinger]

On 19.03.2009 13:25, Peter Stuge wrote:
> Carl-Daniel Hailfinger wrote:
> > This patch tries to make printk more readable and reliable.
>
> I like the idea.
> > Introduce "<almost NULL>" for string arguments with addresses
> > below 0x400.
>
> I would like the message to be more informative. Include the address
> somehow..
>
> "<near-NULL %03x>" or so?
>
> (No, I don't want to make printf() recurse, but you get my idea. :)

I fully agree. I looked at the code repeatedly and I can't find a nice
and readable way to do it. Sure, there are hackish ways to achieve the
goal, but vtxprintf is already complicated enough to let me stop before
such a trick.
"<%03x near NULL>" would be an alternative with mostly readable code.

The new message reads "<near NULL>".
> > The idea is to catch garbage strings from stray pointers and print
> > "<GARBAGE>" instead.
>
> Again request more informative text. For example I think
> "<non-ASCII characters>" would be much nicer.

Changed.
> > Signed-off-by: Carl-Daniel Hailfinger
>
> If you agree with my requests I say
>
> Acked-by: Peter Stuge

I'd like to postpone the near-NULL address printing until someone else
chimes in.

Except for that address printing, your suggestions have been
incorporated. OK to commit?

Regards,
Carl-Daniel

[Peter Stuge]

Carl-Daniel Hailfinger wrote:
> I fully agree. I looked at the code repeatedly and I can't find a nice
> and readable way to do it. Sure, there are hackish ways to achieve the
> goal, but vtxprintf is already complicated enough to let me stop before
> such a trick.
> "<%03x near NULL>" would be an alternative with mostly readable code.

I think this looks nice.
> The new message reads "<near NULL>".

Ok. I'll look at adding the address in a separate patch.
> OK to commit?

Go for it.

//Peter

[Carl-Daniel Hailfinger]

On 19.03.2009 13:47, Peter Stuge wrote:
> Carl-Daniel Hailfinger wrote:
> > I fully agree. I looked at the code repeatedly and I can't find a nice
> > and readable way to do it. Sure, there are hackish ways to achieve the
> > goal, but vtxprintf is already complicated enough to let me stop before
> > such a trick.
> > "<%03x near NULL>" would be an alternative with mostly readable code.
>
> I think this looks nice.
> > The new message reads "<near NULL>".
>
> Ok. I'll look at adding the address in a separate patch.

Thanks.
> > OK to commit?
>
> Go for it.

Committed in r4020.

Regards,
Carl-Daniel

[Pattrick Hueper]

On Thu, Mar 19, 2009 at 1:55 PM, Carl-Daniel Hailfinger
 wrote:
> > > OK to commit?
> >
> > Go for it.
>
> Committed in r4020.

me thinks it was r1158?

Regards, Pattrick

[Mathias Krause]

Carl-Daniel Hailfinger wrote:
> @@ -194,9 +195,15 @@
>    		s = va_arg(args, char *);
>    		if (!s)
>    			s = "<NULL>";
> 	/* Catch almost-NULL pointers as well */
> 	if ((size_t)s < 0x400)
> 		s = "<almost NULL>";
>
> len = strnlen(s, precision);
>
> 	for (i = 0; i < len; ++i)
> 		if (!isprint(*s[i]))
> 			s = "<GARBAGE>";

No offense, but isn't there a break missing in the loop when we assign s
to be "<GARBAGE>"? Otherwise we may access invalid data if len is
greater than strlen("<GARBAGE>") because we have changed the base we're
looking at.
> 	if (!(flags & LEFT))
> 		while (len < field_width--)
> 			tx_byte(' ', arg), count++;

Mathias

[Peter Stuge]

Mathias Krause wrote:
> > 	for (i = 0; i < len; ++i)
> > 		if (!isprint(*s[i]))
> > 			s = "<GARBAGE>";
>
> No offense,

None taken. We very much appreciate your contribution!
> but isn't there a break missing in the loop when we assign s to be
> "<GARBAGE>"?

Yes. Good find. Thank you for spotting it!
> Otherwise we may access invalid data if len is greater than
> strlen("<GARBAGE>") because we have changed the base we're looking
> at.

Yup. r1159

//Peter