[U-Boot] [PATCH v2 0/3] Buffer overruns in printf
The printf family of functions in U-Boot cannot deal with a situation where the caller provides a buffer which turns out to be too small for the format string. This can result in buffer overflows, stack overflows and other bad behavior.
This patch series tidies this up in the common vsprintf.c code.
You can find a discussion of the Linux / U-Boot licensing issues here: http://patchwork.ozlabs.org/patch/116161/
Code Size Impact ----------------
(From Simon Glass sjg@chromium.org) With my ARMv7 compiler (gcc-4.4.3_cos_gg_53174) the code size increase is 328 bytes, about 10% increase to code size vsprintf.o.
The newly added functions (snprintf, vscnprintf, scnprintf) are a total of 116 bytes.
The changes to number(), string() and vsprintf() to make them respect an end pointer increase size by 80, 20 and 80 bytes respectively.
Total text size for existing vsprintf.o functions goes from 0xc10 (3088) to 0xd58 (3416), or 328 bytes. Of this 116 bytes is the new functions and the rest is dealing with the end pointer. There is no data.
Changes in v2: - Use sizeof(printbuffer) instead of CONFIG_SYS_PBSIZE - Drop patch which changes network code to use snprintf()
Simon Glass (1): Add limits.h to hold basic limits
Sonny Rao (2): Add safe vsnprintf and snprintf library functions Make printf and vprintf safe from buffer overruns
common/console.c | 10 +- fs/ubifs/ubifs.h | 4 +- include/common.h | 8 ++- include/limits.h | 40 +++++++ lib/vsprintf.c | 316 ++++++++++++++++++++++++++++++++++++++++++------------ 5 files changed, 302 insertions(+), 76 deletions(-) create mode 100644 include/limits.h
This brings a basic limits.h implementation into U-Boot.
Signed-off-by: Simon Glass sjg@chromium.org --- fs/ubifs/ubifs.h | 4 +--- include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 include/limits.h
diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h index 0af471a..e7b6e43 100644 --- a/fs/ubifs/ubifs.h +++ b/fs/ubifs/ubifs.h @@ -311,9 +311,7 @@ struct file { #define MAX_LFS_FILESIZE 0x7fffffffffffffffUL #endif
-#define INT_MAX ((int)(~0U>>1)) -#define INT_MIN (-INT_MAX - 1) -#define LLONG_MAX ((long long)(~0ULL>>1)) +#include <limits.h>
/* * These are the fs-independent mount-flags: up to 32 flags are supported diff --git a/include/limits.h b/include/limits.h new file mode 100644 index 0000000..1021291 --- /dev/null +++ b/include/limits.h @@ -0,0 +1,40 @@ +/* + * Copyright (c) 2011 The Chromium OS Authors. + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, + * MA 02111-1307 USA + */ + +#ifndef __LIMITS_H +#define __LIMITS_H + +/* Keep all our limits in one place */ + +#define USHRT_MAX ((u16)(~0U)) +#define SHRT_MAX ((s16)(USHRT_MAX>>1)) +#define SHRT_MIN ((s16)(-SHRT_MAX - 1)) +#define INT_MAX ((int)(~0U>>1)) +#define INT_MIN (-INT_MAX - 1) +#define UINT_MAX (~0U) +#define LONG_MAX ((long)(~0UL>>1)) +#define LONG_MIN (-LONG_MAX - 1) +#define ULONG_MAX (~0UL) +#define LLONG_MAX ((long long)(~0ULL>>1)) +#define LLONG_MIN (-LLONG_MAX - 1) +#define ULLONG_MAX (~0ULL) + +#endif
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a écrit :
This brings a basic limits.h implementation into U-Boot.
Signed-off-by: Simon Glasssjg@chromium.org
fs/ubifs/ubifs.h | 4 +--- include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 include/limits.h
Apparently, in all the U-Boot codebase, only one file required standard limit names, and gets them with three lines of code. Is it worth adding 40 lines of code for this?
Amicalement,
Hi Albert,
On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a écrit :
This brings a basic limits.h implementation into U-Boot.
Signed-off-by: Simon Glasssjg@chromium.org
fs/ubifs/ubifs.h | 4 +--- include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 include/limits.h
Apparently, in all the U-Boot codebase, only one file required standard limit names, and gets them with three lines of code. Is it worth adding 40 lines of code for this?
Well 2/3 is the license header - and I thought it best to add all the limits in one go. I can add just those few if you like.
This file is used later in the patch series.
Regards, Simon
Amicalement,
Albert.
Le 21/10/2011 22:19, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a écrit :
This brings a basic limits.h implementation into U-Boot.
Signed-off-by: Simon Glasssjg@chromium.org
fs/ubifs/ubifs.h | 4 +--- include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 include/limits.h
Apparently, in all the U-Boot codebase, only one file required standard limit names, and gets them with three lines of code. Is it worth adding 40 lines of code for this?
Well 2/3 is the license header - and I thought it best to add all the limits in one go. I can add just those few if you like.
This file is used later in the patch series.
I don't see much use of these in the subsequent patches either -- and those few uses could be discussed, such as for instance the use of INT_MAX as the maximum buffer size for some *printf() functions -- actually, anything very big would fit just as well, would it not?
Regards, Simon
Amicalement,
Hi Albert,
On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 22:19, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a écrit :
This brings a basic limits.h implementation into U-Boot.
Signed-off-by: Simon Glasssjg@chromium.org
fs/ubifs/ubifs.h | 4 +--- include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 include/limits.h
Apparently, in all the U-Boot codebase, only one file required standard limit names, and gets them with three lines of code. Is it worth adding 40 lines of code for this?
Well 2/3 is the license header - and I thought it best to add all the limits in one go. I can add just those few if you like.
This file is used later in the patch series.
I don't see much use of these in the subsequent patches either -- and those few uses could be discussed, such as for instance the use of INT_MAX as the maximum buffer size for some *printf() functions -- actually, anything very big would fit just as well, would it not?
Yes it would, it's doesn't really need to be INT_MAX. Then again, limits.h is a fairly standard file to have around, and INT_MAX is an efficient 'large' value to load on many architectures.
In any case it seems wrong to me that ubifs is defining its own INT_MAX and U-Boot doesn't have one.
Regards, Simon
Regards, Simon
Amicalement,
Albert.
Le 21/10/2011 23:12, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 22:19, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a écrit :
This brings a basic limits.h implementation into U-Boot.
Signed-off-by: Simon Glasssjg@chromium.org
fs/ubifs/ubifs.h | 4 +--- include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 include/limits.h
Apparently, in all the U-Boot codebase, only one file required standard limit names, and gets them with three lines of code. Is it worth adding 40 lines of code for this?
Well 2/3 is the license header - and I thought it best to add all the limits in one go. I can add just those few if you like.
This file is used later in the patch series.
I don't see much use of these in the subsequent patches either -- and those few uses could be discussed, such as for instance the use of INT_MAX as the maximum buffer size for some *printf() functions -- actually, anything very big would fit just as well, would it not?
Yes it would, it's doesn't really need to be INT_MAX. Then again, limits.h is a fairly standard file to have around, and INT_MAX is an efficient 'large' value to load on many architectures.
In any case it seems wrong to me that ubifs is defining its own INT_MAX and U-Boot doesn't have one.
My point is precisely that as standard as limits.h is, U-Boot has managed to survive not having it around so far, which kind of shows limits.h is not needed.
Regards, Simon
Amicalement,
Hi Albert,
On Fri, Oct 21, 2011 at 2:47 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 23:12, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 22:19, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a écrit :
This brings a basic limits.h implementation into U-Boot.
Signed-off-by: Simon Glasssjg@chromium.org
fs/ubifs/ubifs.h | 4 +--- include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 41 insertions(+), 3 deletions(-) create mode 100644 include/limits.h
Apparently, in all the U-Boot codebase, only one file required standard limit names, and gets them with three lines of code. Is it worth adding 40 lines of code for this?
Well 2/3 is the license header - and I thought it best to add all the limits in one go. I can add just those few if you like.
This file is used later in the patch series.
I don't see much use of these in the subsequent patches either -- and those few uses could be discussed, such as for instance the use of INT_MAX as the maximum buffer size for some *printf() functions -- actually, anything very big would fit just as well, would it not?
Yes it would, it's doesn't really need to be INT_MAX. Then again, limits.h is a fairly standard file to have around, and INT_MAX is an efficient 'large' value to load on many architectures.
In any case it seems wrong to me that ubifs is defining its own INT_MAX and U-Boot doesn't have one.
My point is precisely that as standard as limits.h is, U-Boot has managed to survive not having it around so far, which kind of shows limits.h is not needed.
By that logic one would never do any code clean ups. Do I understand you correctly?
But this is the least of my concerns :-) If you don't want it, don't take it. Shall I modify the series to define its own INT_MAX, or just chose a large number?
BTW I think you are looking at the old version of that patch series - we are now on v4. The limits.h patch is the same though. Later on in the series I add comments to vsprintf() functions and move them into their own header. If you apply the same logic there then that tidy is not needed also. Please let me know.
Regards, Simon
Regards, Simon
Amicalement,
Albert.
Le 22/10/2011 00:02, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 2:47 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 23:12, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 22:19, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a écrit : > > This brings a basic limits.h implementation into U-Boot. > > Signed-off-by: Simon Glasssjg@chromium.org > --- > fs/ubifs/ubifs.h | 4 +--- > include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 41 insertions(+), 3 deletions(-) > create mode 100644 include/limits.h
Apparently, in all the U-Boot codebase, only one file required standard limit names, and gets them with three lines of code. Is it worth adding 40 lines of code for this?
Well 2/3 is the license header - and I thought it best to add all the limits in one go. I can add just those few if you like.
This file is used later in the patch series.
I don't see much use of these in the subsequent patches either -- and those few uses could be discussed, such as for instance the use of INT_MAX as the maximum buffer size for some *printf() functions -- actually, anything very big would fit just as well, would it not?
Yes it would, it's doesn't really need to be INT_MAX. Then again, limits.h is a fairly standard file to have around, and INT_MAX is an efficient 'large' value to load on many architectures.
In any case it seems wrong to me that ubifs is defining its own INT_MAX and U-Boot doesn't have one.
My point is precisely that as standard as limits.h is, U-Boot has managed to survive not having it around so far, which kind of shows limits.h is not needed.
By that logic one would never do any code clean ups. Do I understand you correctly?
You're extending my logic here: not all cleanups are done by adding a header file and replacing some lines by an include and some other lines. :)
Actually, I don't think introducing limits.h is any cleanup; it is an attempt at using standards whenever possible, which may be fine with some standards: I'd be happy of U-Boot used uint{8,16,32}_t instead of u{8,16,32}, for instance, because it uses that a lot. With limits.h, my gripe with it here is that, while possible, I don't see it bringing benefits here as 1) the ubi code already defines what it needs, 2) other uses in the patchset do not actually require /limits/, and 3) there are not many places in the whole U-Boot code that do.
If you can prove me wrong, i.e. if you can show that limits.h would add a lot of benefits to more than the other files in its own patchset, then I'll happily reconsider.
But this is the least of my concerns :-) If you don't want it, don't take it. Shall I modify the series to define its own INT_MAX, or just chose a large number?
Well I don't think the limits.h introduction is useful here, and not introducing it will barely cost a source code line. As for the number to use in *printf(), either way is fine with me, as this number is arbitrary anyway.
BTW I think you are looking at the old version of that patch series - we are now on v4. The limits.h patch is the same though. Later on in the series I add comments to vsprintf() functions and move them into their own header. If you apply the same logic there then that tidy is not needed also. Please let me know.
Thanks for reminding me. I did see the V4 series and it is the one I actually commented on in my previous mail. Apologies for not having made that explicit.
Regards, Simon
Amicalement,
Hi Albert,
On Fri, Oct 21, 2011 at 3:39 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 22/10/2011 00:02, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 2:47 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 23:12, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 22:19, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote: > > Hi Simon, > > Le 10/10/2011 21:22, Simon Glass a écrit : >> >> This brings a basic limits.h implementation into U-Boot. >> >> Signed-off-by: Simon Glasssjg@chromium.org >> --- >> fs/ubifs/ubifs.h | 4 +--- >> include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 41 insertions(+), 3 deletions(-) >> create mode 100644 include/limits.h > > Apparently, in all the U-Boot codebase, only one file required > standard > limit names, and gets them with three lines of code. Is it worth > adding > 40 > lines of code for this?
Well 2/3 is the license header - and I thought it best to add all the limits in one go. I can add just those few if you like.
This file is used later in the patch series.
I don't see much use of these in the subsequent patches either -- and those few uses could be discussed, such as for instance the use of INT_MAX as the maximum buffer size for some *printf() functions -- actually, anything very big would fit just as well, would it not?
Yes it would, it's doesn't really need to be INT_MAX. Then again, limits.h is a fairly standard file to have around, and INT_MAX is an efficient 'large' value to load on many architectures.
In any case it seems wrong to me that ubifs is defining its own INT_MAX and U-Boot doesn't have one.
My point is precisely that as standard as limits.h is, U-Boot has managed to survive not having it around so far, which kind of shows limits.h is not needed.
By that logic one would never do any code clean ups. Do I understand you correctly?
You're extending my logic here: not all cleanups are done by adding a header file and replacing some lines by an include and some other lines. :)
Actually, I don't think introducing limits.h is any cleanup; it is an attempt at using standards whenever possible, which may be fine with some standards: I'd be happy of U-Boot used uint{8,16,32}_t instead of u{8,16,32}, for instance, because it uses that a lot. With limits.h, my gripe with it here is that, while possible, I don't see it bringing benefits here as 1) the ubi code already defines what it needs, 2) other uses in the patchset do not actually require /limits/, and 3) there are not many places in the whole U-Boot code that do.
If you can prove me wrong, i.e. if you can show that limits.h would add a lot of benefits to more than the other files in its own patchset, then I'll happily reconsider.
I see few and small benefits. Of course if it is not there then people will not use it, so it is self-fulfilling.
But this is the least of my concerns :-) If you don't want it, don't take it. Shall I modify the series to define its own INT_MAX, or just chose a large number?
Well I don't think the limits.h introduction is useful here, and not introducing it will barely cost a source code line. As for the number to use in *printf(), either way is fine with me, as this number is arbitrary anyway.
OK
BTW I think you are looking at the old version of that patch series - we are now on v4. The limits.h patch is the same though. Later on in the series I add comments to vsprintf() functions and move them into their own header. If you apply the same logic there then that tidy is not needed also. Please let me know.
Thanks for reminding me. I did see the V4 series and it is the one I actually commented on in my previous mail. Apologies for not having made that explicit.
OK that's fine - I will redo the series without limits.h.
Regards, Simon
Regards, Simon
Amicalement,
Albert.
Hi Albert,
On Fri, Oct 21, 2011 at 9:58 PM, Simon Glass sjg@chromium.org wrote:
Hi Albert,
On Fri, Oct 21, 2011 at 3:39 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 22/10/2011 00:02, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 2:47 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 23:12, Simon Glass a écrit :
Hi Albert,
On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD albert.u.boot@aribaud.net wrote:
Le 21/10/2011 22:19, Simon Glass a écrit : > > Hi Albert, > > On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD > albert.u.boot@aribaud.net wrote: >> >> Hi Simon, >> >> Le 10/10/2011 21:22, Simon Glass a écrit : >>> >>> This brings a basic limits.h implementation into U-Boot. >>> >>> Signed-off-by: Simon Glasssjg@chromium.org >>> --- >>> fs/ubifs/ubifs.h | 4 +--- >>> include/limits.h | 40 ++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 41 insertions(+), 3 deletions(-) >>> create mode 100644 include/limits.h >> >> Apparently, in all the U-Boot codebase, only one file required >> standard >> limit names, and gets them with three lines of code. Is it worth >> adding >> 40 >> lines of code for this? > > Well 2/3 is the license header - and I thought it best to add all the > limits in one go. I can add just those few if you like. > > This file is used later in the patch series.
I don't see much use of these in the subsequent patches either -- and those few uses could be discussed, such as for instance the use of INT_MAX as the maximum buffer size for some *printf() functions -- actually, anything very big would fit just as well, would it not?
Yes it would, it's doesn't really need to be INT_MAX. Then again, limits.h is a fairly standard file to have around, and INT_MAX is an efficient 'large' value to load on many architectures.
In any case it seems wrong to me that ubifs is defining its own INT_MAX and U-Boot doesn't have one.
My point is precisely that as standard as limits.h is, U-Boot has managed to survive not having it around so far, which kind of shows limits.h is not needed.
By that logic one would never do any code clean ups. Do I understand you correctly?
You're extending my logic here: not all cleanups are done by adding a header file and replacing some lines by an include and some other lines. :)
Actually, I don't think introducing limits.h is any cleanup; it is an attempt at using standards whenever possible, which may be fine with some standards: I'd be happy of U-Boot used uint{8,16,32}_t instead of u{8,16,32}, for instance, because it uses that a lot. With limits.h, my gripe with it here is that, while possible, I don't see it bringing benefits here as 1) the ubi code already defines what it needs, 2) other uses in the patchset do not actually require /limits/, and 3) there are not many places in the whole U-Boot code that do.
If you can prove me wrong, i.e. if you can show that limits.h would add a lot of benefits to more than the other files in its own patchset, then I'll happily reconsider.
I see few and small benefits. Of course if it is not there then people will not use it, so it is self-fulfilling.
But this is the least of my concerns :-) If you don't want it, don't take it. Shall I modify the series to define its own INT_MAX, or just chose a large number?
Well I don't think the limits.h introduction is useful here, and not introducing it will barely cost a source code line. As for the number to use in *printf(), either way is fine with me, as this number is arbitrary anyway.
OK
BTW I think you are looking at the old version of that patch series - we are now on v4. The limits.h patch is the same though. Later on in the series I add comments to vsprintf() functions and move them into their own header. If you apply the same logic there then that tidy is not needed also. Please let me know.
Thanks for reminding me. I did see the V4 series and it is the one I actually commented on in my previous mail. Apologies for not having made that explicit.
OK that's fine - I will redo the series without limits.h.
Done - sent as v5.
Regards, Simon
Regards, Simon
Regards, Simon
Amicalement,
Albert.
On Monday 10 October 2011 15:22:29 Simon Glass wrote:
This brings a basic limits.h implementation into U-Boot.
sorry to jump in so late, but i think this was the correct way to go. copying & pasting the same defines throughout the tree doesn't make much sense. this would also make it easier to grab changes from Linux since they're the same. -mike
Hi Mike
On Thu, Nov 3, 2011 at 7:33 PM, Mike Frysinger vapier@gentoo.org wrote:
On Monday 10 October 2011 15:22:29 Simon Glass wrote:
This brings a basic limits.h implementation into U-Boot.
sorry to jump in so late, but i think this was the correct way to go. copying & pasting the same defines throughout the tree doesn't make much sense. this would also make it easier to grab changes from Linux since they're the same. -mike
Well Albert was pretty keen on leaving this out. Next time we need INT_MAX somewhere, or see someone defining it in a patch, we can think about adding limits.h.
Regards, Simon
On Friday 04 November 2011 01:14:08 Simon Glass wrote:
On Thu, Nov 3, 2011 at 7:33 PM, Mike Frysinger wrote:
On Monday 10 October 2011 15:22:29 Simon Glass wrote:
This brings a basic limits.h implementation into U-Boot.
sorry to jump in so late, but i think this was the correct way to go. copying & pasting the same defines throughout the tree doesn't make much sense. this would also make it easier to grab changes from Linux since they're the same.
Well Albert was pretty keen on leaving this out. Next time we need INT_MAX somewhere, or see someone defining it in a patch, we can think about adding limits.h.
his complaint was that there was one consumer. we now have two. and in both cases, it's because we imported code from Linux.
i would take the position that even if there is just one consumer (ubifs in this case), we should be importing this. although it might make more sense to have this in linux/limits.h. only downside there is that the upstream Linux code (oddly) places these in linux/kernel.h instead of linux/limits.h. but i think i can live with that idiosyncrasy. -mike
Hi Mike,
On Fri, Nov 4, 2011 at 4:09 PM, Mike Frysinger vapier@gentoo.org wrote:
On Friday 04 November 2011 01:14:08 Simon Glass wrote:
On Thu, Nov 3, 2011 at 7:33 PM, Mike Frysinger wrote:
On Monday 10 October 2011 15:22:29 Simon Glass wrote:
This brings a basic limits.h implementation into U-Boot.
sorry to jump in so late, but i think this was the correct way to go. copying & pasting the same defines throughout the tree doesn't make much sense. this would also make it easier to grab changes from Linux since they're the same.
Well Albert was pretty keen on leaving this out. Next time we need INT_MAX somewhere, or see someone defining it in a patch, we can think about adding limits.h.
his complaint was that there was one consumer. we now have two. and in both cases, it's because we imported code from Linux.
My snprintf() patch series added the second so Albert was aware of that. Also note it has not been merged yet.
i would take the position that even if there is just one consumer (ubifs in this case), we should be importing this. although it might make more sense to have this in linux/limits.h. only downside there is that the upstream Linux code (oddly) places these in linux/kernel.h instead of linux/limits.h. but i think i can live with that idiosyncrasy. -mike
Will let you and Albert discuss it over a beer :-)
Regards, Simon
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
These functions are useful in U-Boot because they allow a graceful failure rather than an unpredictable stack overflow when printf() buffers are exceeded.
Mostly copied from the Linux kernel. I copied vscnprintf and scnprintf so we can change printf and vprintf to use the safe implementation but still return the correct values.
Signed-off-by: Sonny Rao sonnyrao@chromium.org --- include/common.h | 8 ++- lib/vsprintf.c | 316 ++++++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 256 insertions(+), 68 deletions(-)
diff --git a/include/common.h b/include/common.h index eb19a44..4e74855 100644 --- a/include/common.h +++ b/include/common.h @@ -699,9 +699,15 @@ unsigned long long simple_strtoull(const char *cp,char **endp,unsigned int base) long simple_strtol(const char *cp,char **endp,unsigned int base); void panic(const char *fmt, ...) __attribute__ ((format (__printf__, 1, 2), noreturn)); -int sprintf(char * buf, const char *fmt, ...) +int sprintf(char *buf, const char *fmt, ...) __attribute__ ((format (__printf__, 2, 3))); +int snprintf(char *buf, size_t size, const char *fmt, ...) + __attribute__ ((format (__printf__, 3, 4))); +int scnprintf(char *buf, size_t size, const char *fmt, ...) + __attribute__ ((format (__printf__, 3, 4))); int vsprintf(char *buf, const char *fmt, va_list args); +int vsnprintf(char *buf, size_t size, const char *fmt, va_list args); +int vscnprintf(char *buf, size_t size, const char *fmt, va_list args);
/* lib/strmhz.c */ char * strmhz(char *buf, unsigned long hz); diff --git a/lib/vsprintf.c b/lib/vsprintf.c index 79dead3..bac6f30 100644 --- a/lib/vsprintf.c +++ b/lib/vsprintf.c @@ -16,6 +16,7 @@ #include <errno.h>
#include <common.h> +#include <limits.h> #if !defined (CONFIG_PANIC_HANG) #include <command.h> #endif @@ -289,7 +290,8 @@ static noinline char* put_dec(char *buf, unsigned NUM_TYPE num) #define SMALL 32 /* Must be 32 == 0x20 */ #define SPECIAL 64 /* 0x */
-static char *number(char *buf, unsigned NUM_TYPE num, int base, int size, int precision, int type) +static char *number(char *buf, char *end, unsigned NUM_TYPE num, + int base, int size, int precision, int type) { /* we are called with base 8, 10 or 16, only, thus don't need "G..." */ static const char digits[16] = "0123456789ABCDEF"; /* "GHIJKLMNOPQRSTUVWXYZ"; */ @@ -351,37 +353,63 @@ static char *number(char *buf, unsigned NUM_TYPE num, int base, int size, int pr precision = i; /* leading space padding */ size -= precision; - if (!(type & (ZEROPAD+LEFT))) - while(--size >= 0) - *buf++ = ' '; + if (!(type & (ZEROPAD+LEFT))) { + while (--size >= 0) { + if (buf < end) + *buf = ' '; + ++buf; + } + } /* sign */ - if (sign) - *buf++ = sign; + if (sign) { + if (buf < end) + *buf = sign; + ++buf; + } /* "0x" / "0" prefix */ if (need_pfx) { - *buf++ = '0'; - if (base == 16) - *buf++ = ('X' | locase); + if (buf < end) + *buf = '0'; + ++buf; + if (base == 16) { + if (buf < end) + *buf = ('X' | locase); + ++buf; + } } /* zero or space padding */ if (!(type & LEFT)) { char c = (type & ZEROPAD) ? '0' : ' '; - while (--size >= 0) - *buf++ = c; + while (--size >= 0) { + if (buf < end) + *buf = c; + ++buf; + } } /* hmm even more zero padding? */ - while (i <= --precision) - *buf++ = '0'; + while (i <= --precision) { + if (buf < end) + *buf = '0'; + ++buf; + } /* actual digits of result */ - while (--i >= 0) - *buf++ = tmp[i]; + while (--i >= 0) { + if (buf < end) + *buf = tmp[i]; + ++buf; + + } /* trailing space padding */ - while (--size >= 0) - *buf++ = ' '; + while (--size >= 0) { + if (buf < end) + *buf = ' '; + ++buf; + } return buf; }
-static char *string(char *buf, char *s, int field_width, int precision, int flags) +static char *string(char *buf, char *end, char *s, int field_width, + int precision, int flags) { int len, i;
@@ -391,17 +419,26 @@ static char *string(char *buf, char *s, int field_width, int precision, int flag len = strnlen(s, precision);
if (!(flags & LEFT)) - while (len < field_width--) - *buf++ = ' '; - for (i = 0; i < len; ++i) - *buf++ = *s++; - while (len < field_width--) - *buf++ = ' '; + while (len < field_width--) { + if (buf < end) + *buf = ' '; + ++buf; + } + for (i = 0; i < len; ++i) { + if (buf < end) + *buf = *s++; + ++buf; + } + while (len < field_width--) { + if (buf < end) + *buf = ' '; + ++buf; + } return buf; }
#ifdef CONFIG_CMD_NET -static char *mac_address_string(char *buf, u8 *addr, int field_width, +static char *mac_address_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char mac_addr[6 * 3]; /* (6 * 2 hex digits), 5 colons and trailing zero */ @@ -415,10 +452,11 @@ static char *mac_address_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, mac_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, mac_addr, field_width, precision, + flags & ~SPECIAL); }
-static char *ip6_addr_string(char *buf, u8 *addr, int field_width, +static char *ip6_addr_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char ip6_addr[8 * 5]; /* (8 * 4 hex digits), 7 colons and trailing zero */ @@ -433,10 +471,11 @@ static char *ip6_addr_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, ip6_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, ip6_addr, field_width, precision, + flags & ~SPECIAL); }
-static char *ip4_addr_string(char *buf, u8 *addr, int field_width, +static char *ip4_addr_string(char *buf, char *end, u8 *addr, int field_width, int precision, int flags) { char ip4_addr[4 * 4]; /* (4 * 3 decimal digits), 3 dots and trailing zero */ @@ -454,7 +493,8 @@ static char *ip4_addr_string(char *buf, u8 *addr, int field_width, } *p = '\0';
- return string(buf, ip4_addr, field_width, precision, flags & ~SPECIAL); + return string(buf, end, ip4_addr, field_width, precision, + flags & ~SPECIAL); } #endif
@@ -476,10 +516,12 @@ static char *ip4_addr_string(char *buf, u8 *addr, int field_width, * function pointers are really function descriptors, which contain a * pointer to the real address. */ -static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int precision, int flags) +static char *pointer(const char *fmt, char *buf, char *end, void *ptr, + int field_width, int precision, int flags) { if (!ptr) - return string(buf, "(null)", field_width, precision, flags); + return string(buf, end, "(null)", field_width, precision, + flags);
#ifdef CONFIG_CMD_NET switch (*fmt) { @@ -487,15 +529,18 @@ static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int flags |= SPECIAL; /* Fallthrough */ case 'M': - return mac_address_string(buf, ptr, field_width, precision, flags); + return mac_address_string(buf, end, ptr, field_width, + precision, flags); case 'i': flags |= SPECIAL; /* Fallthrough */ case 'I': if (fmt[1] == '6') - return ip6_addr_string(buf, ptr, field_width, precision, flags); + return ip6_addr_string(buf, end, ptr, field_width, + precision, flags); if (fmt[1] == '4') - return ip4_addr_string(buf, ptr, field_width, precision, flags); + return ip4_addr_string(buf, end, ptr, field_width, + precision, flags); flags &= ~SPECIAL; break; } @@ -505,31 +550,35 @@ static char *pointer(const char *fmt, char *buf, void *ptr, int field_width, int field_width = 2*sizeof(void *); flags |= ZEROPAD; } - return number(buf, (unsigned long) ptr, 16, field_width, precision, flags); + return number(buf, end, (unsigned long)ptr, 16, field_width, + precision, flags); }
/** - * vsprintf - Format a string and place it in a buffer - * @buf: The buffer to place the result into - * @fmt: The format string to use - * @args: Arguments for the format string + * Format a string and place it in a buffer (base function) * - * This function follows C99 vsprintf, but has some extensions: + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * + * This function follows C99 vsnprintf, but has some extensions: * %pS output the name of a text symbol * %pF output the name of a function pointer * %pR output the address range in a struct resource * - * The function returns the number of characters written - * into @buf. + * The function returns the number of characters which would be + * generated for the given input, excluding the trailing '\0', + * as per ISO C99. * * Call this function if you are already dealing with a va_list. - * You probably want sprintf() instead. + * You probably want snprintf() instead. */ -int vsprintf(char *buf, const char *fmt, va_list args) +int vsnprintf(char *buf, size_t size, const char *fmt, va_list args) { unsigned NUM_TYPE num; int base; - char *str; + char *str, *end;
int flags; /* flags to number() */
@@ -542,10 +591,19 @@ int vsprintf(char *buf, const char *fmt, va_list args) /* 't' added for ptrdiff_t */
str = buf; + end = str + size; + + /* Make sure end is always >= buf */ + if (end < buf) { + end = ((void *)-1); + size = end - buf; + }
for (; *fmt ; ++fmt) { if (*fmt != '%') { - *str++ = *fmt; + if (str < end) + *str = *fmt; + ++str; continue; }
@@ -606,21 +664,32 @@ int vsprintf(char *buf, const char *fmt, va_list args) base = 10;
switch (*fmt) { - case 'c': + case 'c': { + char c; if (!(flags & LEFT)) - while (--field_width > 0) - *str++ = ' '; - *str++ = (unsigned char) va_arg(args, int); - while (--field_width > 0) - *str++ = ' '; + while (--field_width > 0) { + if (str < end) + *str = ' '; + ++str; + } + c = (unsigned char) va_arg(args, int); + if (str < end) + *str = c; + ++str; + while (--field_width > 0) { + if (str < end) + *str = ' '; + ++str; + } continue; - + } case 's': - str = string(str, va_arg(args, char *), field_width, precision, flags); + str = string(str, end, va_arg(args, char *), + field_width, precision, flags); continue;
case 'p': - str = pointer(fmt+1, str, + str = pointer(fmt+1, str, end, va_arg(args, void *), field_width, precision, flags); /* Skip all alphanumeric pointer suffixes */ @@ -639,7 +708,9 @@ int vsprintf(char *buf, const char *fmt, va_list args) continue;
case '%': - *str++ = '%'; + if (str < end) + *str = '%'; + ++str; continue;
/* integer number formats - set up the flags and "break" */ @@ -660,10 +731,14 @@ int vsprintf(char *buf, const char *fmt, va_list args) break;
default: - *str++ = '%'; - if (*fmt) - *str++ = *fmt; - else + if (str < end) + *str = '%'; + ++str; + if (*fmt) { + if (str < end) + *str = *fmt; + ++str; + } else --fmt; continue; } @@ -686,17 +761,124 @@ int vsprintf(char *buf, const char *fmt, va_list args) if (flags & SIGN) num = (signed int) num; } - str = number(str, num, base, field_width, precision, flags); + str = number(str, end, num, base, field_width, precision, + flags); + } + + if (size > 0) { + if (str < end) + *str = '\0'; + else + end[-1] = '\0'; } - *str = '\0'; + /* the trailing null byte doesn't count towards the total */ return str-buf; }
/** - * sprintf - Format a string and place it in a buffer - * @buf: The buffer to place the result into - * @fmt: The format string to use - * @...: Arguments for the format string + * Format a string and place it in a buffer (va_list version) + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param args Arguments for the format string + * @return the number of characters which have been written into + * the @buf not including the trailing '\0'. If @size is == 0 the function + * returns 0. + * + * If you're not already dealing with a va_list consider using scnprintf(). + * + * See the vsprintf() documentation for format string extensions over C99. + */ +int vscnprintf(char *buf, size_t size, const char *fmt, va_list args) +{ + int i; + + i = vsnprintf(buf, size, fmt, args); + + if (likely(i < size)) + return i; + if (size != 0) + return size - 1; + return 0; +} + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param ... Arguments for the format string + * @return the number of characters which would be + * generated for the given input, excluding the trailing null, + * as per ISO C99. If the return is greater than or equal to + * @size, the resulting string is truncated. + * + * See the vsprintf() documentation for format string extensions over C99. + */ +int snprintf(char *buf, size_t size, const char *fmt, ...) +{ + va_list args; + int i; + + va_start(args, fmt); + i = vsnprintf(buf, size, fmt, args); + va_end(args); + + return i; +} + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param size The size of the buffer, including the trailing null space + * @param fmt The format string to use + * @param ... Arguments for the format string + * + * The return value is the number of characters written into @buf not including + * the trailing '\0'. If @size is == 0 the function returns 0. + * + * See the vsprintf() documentation for format string extensions over C99. + */ + +int scnprintf(char *buf, size_t size, const char *fmt, ...) +{ + va_list args; + int i; + + va_start(args, fmt); + i = vscnprintf(buf, size, fmt, args); + va_end(args); + + return i; +} + +/** + * Format a string and place it in a buffer (va_list version) + * + * @param buf The buffer to place the result into + * @param fmt The format string to use + * @param args Arguments for the format string + * + * The function returns the number of characters written + * into @buf. Use vsnprintf() or vscnprintf() in order to avoid + * buffer overflows. + * + * If you're not already dealing with a va_list consider using sprintf(). + */ +int vsprintf(char *buf, const char *fmt, va_list args) +{ + return vsnprintf(buf, INT_MAX, fmt, args); +} + +/** + * Format a string and place it in a buffer + * + * @param buf The buffer to place the result into + * @param fmt The format string to use + * @param ... Arguments for the format string * * The function returns the number of characters written * into @buf.
From: Sonny Rao sonnyrao@chromium.org
From: Sonny Rao sonnyrao@chromium.org
utilize the added vscnprintf functions to avoid buffer overruns The implementation is fairly dumb in that it doesn't detect that the buffer is too small, but at least will not cause crashes.
Signed-off-by: Sonny Rao sonnyrao@chromium.org --- Changes in v2: - Use sizeof(printbuffer) instead of CONFIG_SYS_PBSIZE - Drop patch which changes network code to use snprintf()
common/console.c | 10 +++++----- 1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/common/console.c b/common/console.c index f17875e..1177f7d 100644 --- a/common/console.c +++ b/common/console.c @@ -212,7 +212,7 @@ int serial_printf(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vscnprintf(printbuffer, sizeof(printbuffer), fmt, args); va_end(args);
serial_puts(printbuffer); @@ -281,7 +281,7 @@ int fprintf(int file, const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vscnprintf(printbuffer, sizeof(printbuffer), fmt, args); va_end(args);
/* Send to desired file */ @@ -426,7 +426,7 @@ int printf(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vscnprintf(printbuffer, sizeof(printbuffer), fmt, args); va_end(args);
/* Print the string */ @@ -447,7 +447,7 @@ int vprintf(const char *fmt, va_list args) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vscnprintf(printbuffer, sizeof(printbuffer), fmt, args);
/* Print the string */ puts(printbuffer); @@ -514,7 +514,7 @@ inline void dbg(const char *fmt, ...) /* For this to work, printbuffer must be larger than * anything we ever want to print. */ - i = vsprintf(printbuffer, fmt, args); + i = vsnprintf(printbuffer, sizeof(printbuffer), fmt, args); va_end(args);
if ((screen + sizeof(screen) - 1 - cursor)
participants (3)
-
Albert ARIBAUD -
Mike Frysinger -
Simon Glass