Buffer overruns in printf

PATCH Superseded Simon Glass 3 participants 17 messages Oct 10, 2011 – Nov 5, 2011

Subsystems (3)

Library code UBI file system (UBIFS) Vsprintf

lore

Share & tools

lore

Simon Glass

author

2011-10-10 19:22 UTC #0 lore raw

v2 cover 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 <[email protected]>)
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

Signature · 1 line

1.7.3.1

Simon Glass

author

2011-10-10 19:22 UTC #1 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

This brings a basic limits.h implementation into U-Boot.

Signed-off-by Simon Glass <[email protected]>

---
 fs/ubifs/ubifs.h |    4 +---
 include/limits.h |   40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 3 deletions(-)
 create mode 100644 include/limits.h

fs/ubifs/ubifs.h +1 -3

index 0af471a..e7b6e43 100644
@@ -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

include/limits.h new file +40 -0

new file mode 100644
index 0000000..1021291
@@ -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

Signature · 1 line

1.7.3.1

Albert ARIBAUD

2011-10-21 19:54 UTC #2 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Hi Simon,

Le 10/10/2011 21:22, Simon Glass a ?crit :

Quoted 8 lines

This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
  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,

Signature · 1 line

Albert.

Simon Glass

author

2011-10-21 20:19 UTC #3 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Hi Albert,

Quoted 13 lines · Albert ARIBAUD

Hi Simon,
Le 10/10/2011 21:22, Simon Glass a ?crit :
Quoted 9 lines
This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
?fs/ubifs/ubifs.h | ? ?4 +---
?include/limits.h | ? 40 ++++++++++++++++++++++++++++++++++++++++ ?2 files changed, 41 insertions(+), 3 deletions(-) ?create mode 100644 include/limits.h

Quoted 3 lines

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

Quoted 5 lines

Amicalement,
--
Albert.

Albert ARIBAUD

2011-10-21 21:00 UTC #4 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Le 21/10/2011 22:19, Simon Glass a ?crit :

Quoted 17 lines

Hi Albert,
Quoted 13 lines · Albert ARIBAUD
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a ?crit :
Quoted 9 lines
This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
  fs/ubifs/ubifs.h |    4 +---
  include/limits.h |   40 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 41 insertions(+), 3 deletions(-)
create mode 100644 include/limits.h

Quoted 8 lines

Quoted 3 lines
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?

Quoted 2 lines

Regards,
Simon

Amicalement,

Signature · 1 line

Albert.

Simon Glass

author

2011-10-21 21:12 UTC #5 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Hi Albert,

Quoted 28 lines · Albert ARIBAUD

Le 21/10/2011 22:19, Simon Glass a ?crit :
Quoted 27 lines
Hi Albert,
Quoted 14 lines · On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD <[email protected]>…
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a ?crit :
Quoted 9 lines
This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
?fs/ubifs/ubifs.h | ? ?4 +---
?include/limits.h | ? 40 ++++++++++++++++++++++++++++++++++++++++ ?2 files changed, 41 insertions(+), 3 deletions(-) ?create mode 100644 include/limits.h
Quoted 4 lines
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.

Quoted 6 lines

Quoted 1 line
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

Quoted 8 lines

Quoted 3 lines
Regards,
Simon
Amicalement,
--
Albert.

Albert ARIBAUD

2011-10-21 21:47 UTC #6 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Le 21/10/2011 23:12, Simon Glass a ?crit :

Quoted 39 lines

Hi Albert,
Quoted 28 lines · Albert ARIBAUD
Le 21/10/2011 22:19, Simon Glass a ?crit :
Quoted 27 lines
Hi Albert,
Quoted 14 lines · Albert ARIBAUD
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a ?crit :
Quoted 9 lines
This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
  fs/ubifs/ubifs.h |    4 +---
  include/limits.h |   40 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 41 insertions(+), 3 deletions(-)
create mode 100644 include/limits.h
Quoted 4 lines
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.
Quoted 6 lines
Quoted 1 line
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?

Quoted 6 lines

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.

Quoted 2 lines

Regards,
Simon

Amicalement,

Signature · 1 line

Albert.

Simon Glass

author

2011-10-21 22:02 UTC #7 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Hi Albert,

Quoted 49 lines · Albert ARIBAUD

Le 21/10/2011 23:12, Simon Glass a ?crit :
Quoted 48 lines
Hi Albert,
Quoted 31 lines · On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD <[email protected]>…
Le 21/10/2011 22:19, Simon Glass a ?crit :
Quoted 24 lines
Hi Albert,
Quoted 14 lines · On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD <[email protected]> ?…
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a ?crit :
Quoted 9 lines
This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
?fs/ubifs/ubifs.h | ? ?4 +---
?include/limits.h | ? 40 ++++++++++++++++++++++++++++++++++++++++ ?2 files changed, 41 insertions(+), 3 deletions(-) ?create mode 100644 include/limits.h
Quoted 4 lines
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?
Quoted 4 lines
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.
Quoted 7 lines
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.

Quoted 6 lines

Quoted 2 lines
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

Quoted 8 lines

Quoted 3 lines
Regards,
Simon
Amicalement,
--
Albert.

Albert ARIBAUD

2011-10-21 22:39 UTC #8 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Le 22/10/2011 00:02, Simon Glass a ?crit :

Quoted 56 lines

Hi Albert,
Quoted 52 lines · Albert ARIBAUD
Le 21/10/2011 23:12, Simon Glass a ?crit :
Quoted 50 lines
Hi Albert,
Quoted 31 lines · Albert ARIBAUD
Le 21/10/2011 22:19, Simon Glass a ?crit :
Quoted 24 lines
Hi Albert,
Quoted 14 lines · Albert ARIBAUD
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a ?crit :
Quoted 9 lines
This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
  fs/ubifs/ubifs.h |    4 +---
  include/limits.h |   40 ++++++++++++++++++++++++++++++++++++++++
  2 files changed, 41 insertions(+), 3 deletions(-)
create mode 100644 include/limits.h
Quoted 4 lines
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?
Quoted 4 lines
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.
Quoted 7 lines
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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Quoted 2 lines

Regards,
Simon

Amicalement,

Signature · 1 line

Albert.

Simon Glass

author

2011-10-22 04:58 UTC #9 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Hi Albert,

Quoted 81 lines · Albert ARIBAUD

Le 22/10/2011 00:02, Simon Glass a ?crit :
Quoted 60 lines
Hi Albert,
Quoted 55 lines · On Fri, Oct 21, 2011 at 2:47 PM, Albert ARIBAUD <[email protected]>…
Le 21/10/2011 23:12, Simon Glass a ?crit :
Quoted 52 lines
Hi Albert,
Quoted 33 lines · On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD <[email protected]> ?…
Le 21/10/2011 22:19, Simon Glass a ?crit :
Quoted 26 lines
Hi Albert,
Quoted 14 lines · On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD <[email protected]> ?…
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a ?crit :
Quoted 9 lines
This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
?fs/ubifs/ubifs.h | ? ?4 +---
?include/limits.h | ? 40 ++++++++++++++++++++++++++++++++++++++++ ?2 files changed, 41 insertions(+), 3 deletions(-) ?create mode 100644 include/limits.h
Quoted 6 lines
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?
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
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.

Quoted 3 lines

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.

Quoted 9 lines

Quoted 4 lines
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.

Quoted 10 lines

Quoted 6 lines
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

Quoted 8 lines

Quoted 3 lines
Regards,
Simon
Amicalement,
--
Albert.

Simon Glass

author

2011-10-25 23:43 UTC #10 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Hi Albert,

Quoted 111 lines · Simon Glass

Hi Albert,
Quoted 81 lines · Albert ARIBAUD
Le 22/10/2011 00:02, Simon Glass a ?crit :
Quoted 60 lines
Hi Albert,
Quoted 55 lines · On Fri, Oct 21, 2011 at 2:47 PM, Albert ARIBAUD <[email protected]>…
Le 21/10/2011 23:12, Simon Glass a ?crit :
Quoted 52 lines
Hi Albert,
Quoted 33 lines · On Fri, Oct 21, 2011 at 2:00 PM, Albert ARIBAUD <[email protected]> ?…
Le 21/10/2011 22:19, Simon Glass a ?crit :
Quoted 26 lines
Hi Albert,
Quoted 14 lines · On Fri, Oct 21, 2011 at 12:54 PM, Albert ARIBAUD <[email protected]> ?…
Hi Simon,
Le 10/10/2011 21:22, Simon Glass a ?crit :
Quoted 9 lines
This brings a basic limits.h implementation into U-Boot.
Signed-off-by Simon Glass<[email protected]>
---
?fs/ubifs/ubifs.h | ? ?4 +---
?include/limits.h | ? 40 ++++++++++++++++++++++++++++++++++++++++ ?2 files changed, 41 insertions(+), 3 deletions(-) ?create mode 100644 include/limits.h
Quoted 6 lines
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?
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 3 lines
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.
Quoted 9 lines
Quoted 4 lines
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
Quoted 7 lines
Quoted 6 lines
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.

Quoted 5 lines

Quoted 3 lines
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

Quoted 13 lines

Regards,
Simon
Quoted 8 lines
Quoted 3 lines
Regards,
Simon
Amicalement,
--
Albert.

Mike Frysinger

2011-11-04 02:33 UTC #11 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

In reply to Simon Glass

Quoted 1 line · 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

Simon Glass

author

2011-11-04 05:14 UTC #12 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Hi Mike

Quoted 8 lines · Mike Frysinger

Quoted 1 line · 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

Mike Frysinger

2011-11-04 23:09 UTC #13 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Quoted 4 lines · On Friday 04 November 2011 01:14:08 Simon Glass wrote:

Quoted 3 lines · Mike Frysinger
Quoted 1 line · On Monday 10 October 2011 15:22:29 Simon Glass wrote:
This brings a basic limits.h implementation into U-Boot.

Quoted 8 lines

Quoted 4 lines
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

Simon Glass

author

2011-11-05 00:24 UTC #14 lore raw

PATCH v2 1/3 Add limits.h to hold basic limits

Hi Mike,

Quoted 10 lines · Mike Frysinger

Quoted 9 lines · On Friday 04 November 2011 01:14:08 Simon Glass wrote:
Quoted 7 lines · Mike Frysinger
Quoted 1 line · 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.

Quoted 6 lines

Quoted 3 lines
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.

Quoted 8 lines

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

Simon Glass

author

2011-10-10 19:22 UTC #15 lore raw

PATCH v2 2/3 Add safe vsnprintf and snprintf library functions

In reply to Simon Glass

From: Sonny Rao <[email protected]>

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 <[email protected]>

---
 include/common.h |    8 ++-
 lib/vsprintf.c   |  316 ++++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 256 insertions(+), 68 deletions(-)

include/common.h +7 -1

index eb19a44..4e74855 100644
@@ -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);

lib/vsprintf.c +249 ~~-67~~

464 lines not on this page · raw message

Signature · 1 line

1.7.3.1

Simon Glass

author

2011-10-10 19:22 UTC #16 lore raw

PATCH v2 3/3 Make printf and vprintf safe from buffer overruns

In reply to Simon Glass

From: Sonny Rao <[email protected]>

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 <[email protected]>

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

common/console.c +5 -5

index f17875e..1177f7d 100644
@@ -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)

Signature · 1 line

1.7.3.1