[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v2] vsprintf: Make sure argument to %pX specifier is valid
On 02/12/2015 10:21 AM, Andrew Cooper wrote:
On 12/02/15 15:01, Boris Ostrovsky wrote:
On 02/12/2015 06:04 AM, Andrew Cooper wrote:
On 11/02/15 20:58, Boris Ostrovsky wrote:
If invalid pointer (i.e. something smaller than HYPERVISOR_VIRT_START)
is passed for %*ph/%pv/%ps/%pS format specifiers then print "(NULL)"
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
---
xen/common/vsprintf.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
v2:
* Print "(NULL)" instead of specifier-specific string
* Consider all addresses under HYPERVISOR_VIRT_START as invalid.
(I think
this is true for both x86 and ARM but I don't have ARM platform
to test).
diff --git a/xen/common/vsprintf.c b/xen/common/vsprintf.c
index 065cc42..b9542b5 100644
--- a/xen/common/vsprintf.c
+++ b/xen/common/vsprintf.c
@@ -270,6 +270,22 @@ static char *pointer(char *str, char *end,
const char **fmt_ptr,
const char *fmt = *fmt_ptr, *s;
/* Custom %p suffixes. See
XEN_ROOT/docs/misc/printk-formats.txt */
+
+ switch ( fmt[1] )
+ {
+ case 'h':
+ case 's':
+ case 'S':
+ case 'v':
+ ++*fmt_ptr;
+ }
+
+ if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
+ {
+ char *s = "(NULL)";
+ return string(str, end, s, -1, -1, 0);
+ }
+
switch ( fmt[1] )
This wont function, as you have inverted the increment of *fmt_ptr and
check of fmt[1].
fmt value doesn't change, it is stashed at the top of the routine.
You are correct. My apologies. I however dislike the splitting of the
switch into two.
(What *is* wrong in the above code is the fact that the arg test is
done outside the switch. It should be part of the four case
statements, otherwise we will print plain %p arguments as "NULL").
"(NULL)" is inappropriate for non-null pointers less than VIRT_START.
Yes, I thought about it after I sent it. "(invalid)"?
Better, but overriding the number with a string does hide information.
In the case that the pointer is invalid, it would be useful to see its
contents.
How about "<0xXXXXXX>" (i.e. effectively replace "%pv" with "<%p>", with
angle brackets indicating invalid pointer)?
Given the VIRT check, I would just put the entire switch statement
inside an "if ( (unsigned long)arg < HYPERVISOR_VIRT_START )" block and
let it fall through to the plain number case for a bogus pointer.
Not sure I understand what you are suggesting here, sorry.
-boris
if ( (unsigned long)arg < HYPERVISOR_VIRT_START )
{
switch ( fmt[1] )
{
....
}
}
This makes the patch a whole 3 line addition and indenting the whole
switch block by 4 spaces.
Still don't understand. This will never print anything unless it's a bad
pointer, won't it?
(And if you meant '>=' then we will simply print the invalid pointer in
plain %p format. Which, btw, may be the solution but we will still need
to bump fmt_ptr, so we again will need another switch or something to
test for sub-specifier)
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
