[U-Boot] [PATCH v2 0/3] Buffer overruns in printf

[U-Boot] [PATCH v2 0/3] Buffer overruns in printf

[Simon Glass]

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

-- 
1.7.3.1

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Simon Glass]

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
-- 
1.7.3.1

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Albert ARIBAUD]

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 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

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,
-- 
Albert.

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Simon Glass]

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 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
>
> 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.
>

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Albert ARIBAUD]

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 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
>>
>> 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,
-- 
Albert.

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Simon Glass]

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 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
>>>
>>> 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.
>

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Albert ARIBAUD]

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 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
>>>>
>>>> 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,
-- 
Albert.

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Simon Glass]

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 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
>>>>>
>>>>> 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.
>

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Albert ARIBAUD]

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 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
>>>>>>
>>>>>> 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,
-- 
Albert.

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Simon Glass]

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 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
>>>>>>>
>>>>>>> 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.
>

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Simon Glass]

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 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
>>>>>>>>
>>>>>>>> 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.
>>
>

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111103/bd24884e/attachment.pgp 

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Simon Glass]

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

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Mike Frysinger]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111104/9d5bdc43/attachment.pgp 

[U-Boot] [PATCH v2 1/3] Add limits.h to hold basic limits

[Simon Glass]

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

[U-Boot] [PATCH v2 2/3] Add safe vsnprintf and snprintf library functions

[Simon Glass]

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.
-- 
1.7.3.1

[U-Boot] [PATCH v2 3/3] Make printf and vprintf safe from buffer overruns

[Simon Glass]

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)
-- 
1.7.3.1