Buffer overruns in printf

From: Sonny Rao <[email protected]>

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.

Hi Simon,

Quoted 8 lines · Simon Glass

From: Sonny Rao <[email protected]>

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.

Quoted 3 lines

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.

Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of Linux is GPLv2 - You may not be legally permitted to do this

Quoted 15 lines · Graeme Russ

Hi Simon,

Quoted 11 lines · Simon Glass

From: Sonny Rao <[email protected]>

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.

Quoted 2 lines

Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of Linux is GPLv2 - You may not be legally permitted to do this

According to the FSF site, GPLv2 is compatible with GPLv3, see:
http://www.gnu.org/licenses/quick-guide-gplv3.html

So it's fine to distribute them together.

In reality though, this code in U-boot was already copied from the same file in an older version of the kernel. The license (GPLv2 only) hasn't changed on that file, so U-boot is already distributing what is GPLv2 only code alongside GPLv2+ code -- which as I mentioned above is fine.

The code here is derived from a later version of that same file, so I don't believe integrating this patch into U-boot actually changes anything with respect to licensing of this code.

Hi Sonny,

Quoted 16 lines · Sonny Rao

Quoted 15 lines · Graeme Russ

Hi Simon,

Quoted 11 lines · Simon Glass

From: Sonny Rao <[email protected]>

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.

Quoted 7 lines

Quoted 2 lines

Have you checked for license compatibility? U-Boot is GPLv2+ and (most) of Linux is GPLv2 - You may not be legally permitted to do this

According to the FSF site, ?GPLv2 is compatible with GPLv3, see:
http://www.gnu.org/licenses/quick-guide-gplv3.html

So it's fine to distribute them together.

Not so fast:

"GPLv2 is compatible with GPLv3 if the program allows you to choose "any later version" of the GPL, which is the case for most software released under this license"

'most', not 'all'

The key phrase is "any later version" - If this is not in the GPLv2 license text for the code your copying, you cannot license the derivative work under GPLv3 (or GPLv2+)

Quoted 5 lines

In reality though, this code in U-boot was already copied from the same file in an older version of the kernel. ?The license (GPLv2 only) hasn't changed on that file, so U-boot is already distributing what is GPLv2 only code alongside GPLv2+ code -- which as I mentioned above is fine.

As long as you are not incorporating GPLv2 code into a file licensed under GPLv2+ and distributing the result as GPLv2+ or GPLv3.

Quoted 3 lines

The code here is derived from a later version of that same file, so I don't believe integrating this patch into U-boot actually changes anything with respect to licensing of this code.

I think U-Boot will have great difficulty going past GPLv2 due to the large volume of Linus code already in U-Boot - We would need to either get the original code relicensed GPLv2+ or rewrite it

And from the COPYING file in the Linux source:

Also note that the only valid version of the GPL as far as the kernel is concerned is _this_ particular version of the license (ie v2, not v2.2 or v3.x or whatever), unless explicitly otherwise stated.

and linux/lib/vsprintf.c:

/*
?* ?linux/lib/vsprintf.c
?*
?* ?Copyright (C) 1991, 1992 ?Linus Torvalds
?*/

/* vsprintf.c -- Lars Wirzenius & Linus Torvalds. */ /*
?* Wirzenius wrote this portably, Torvalds fucked it up :-) ?*/

/*
?* Fri Jul 13 2001 Crutcher Dunnavant <[email protected]> ?* - changed to provide snprintf and vsnprintf functions
?* So Feb ?1 16:51:32 CET 2004 Juergen Quade <[email protected]> ?* - scnprintf and vscnprintf
?*/

No 'any later version'

Now U-Boot COPYING has:

U-Boot is Free Software. ?It is copyrighted by Wolfgang Denk and many others who contributed code (see the actual source code for details). ?You can redistribute U-Boot and/or modify it under the terms of version 2 of the GNU General Public License as published by the Free Software Foundation. ?Most of it can also be distributed, ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ----------------------------------- at your option, under any later version of the GNU General Public
-----------------------------------------------------------------
License -- see individual files for exceptions.
-----------------------------------------------

So U-Boot is also GPLv2, but parts (like Linux) are GPLv2+

My point was there is a long term 'vision' for U-Boot to go GPLv3, and part of that vision is to reject any future GPLv2 only licensed code submitted. If we want the vision to be realised, I think we need to look at how we build new GPLv2+ code in new files rather than tying new code into GPLv2 only files which may, potentially, lock that code down. Although as the writer and copyright holder of a modification you may be free to move it over at a later date anyway - I don't know, and I think that's in the 'ask a lawyer' territory.

So yes, you are right - In terms of license compliance, we are OK as U-Boot is currently GPLv2 - In terms of U-Boot going to GPLv3, we are digging a bigger hole :)

Quoted 2 lines · Graeme Russ

Hi Sonny,

Quoted 3 lines

So yes, you are right - In terms of license compliance, we are OK as U-Boot is currently GPLv2 - In terms of U-Boot going to GPLv3, we are digging a bigger hole :)

Ok, then that's up to the U-boot community to decide if they wish to remove all GPLv2 only code, but I think that's a separate issue from what you originally brought up. I think we're in agreement that there aren't any licensing considerations with respect to maintaining the existing license which is GPLv2 only for lib/vsprintf.c.

From: Sonny Rao <[email protected]>

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

---
common/console.c | 10 +++++-----

1 files changed, 5 insertions(+), 5 deletions(-)

Quoted 8 lines · Simon Glass

From: Sonny Rao <[email protected]>

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.

Quoted 4 lines

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

---
common/console.c | ? 10 +++++-----

1 files changed, 5 insertions(+), 5 deletions(-)

If these are from Sonny, they really should have a Signed-off-by from him.

- k

Quoted 8 lines · On Friday, September 23, 2011 13:38:51 Simon Glass wrote:

common/console.c +0 -0

@@ -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, CONFIG_SYS_PBSIZE, fmt, args);

i think sizeof(printbuffer) would be better. ?same goes for all the other changes here.
-mike

Quoted 13 lines · Mike Frysinger

common/console.c int serial_printf(const char *fmt, ...) · line 215

Quoted 8 lines · On Friday, September 23, 2011 13:38:51 Simon Glass wrote:

common/console.c +1 -1

@@ -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, CONFIG_SYS_PBSIZE, fmt, args);

i think sizeof(printbuffer) would be better. same goes for all the other changes here.
-mike

Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?

Quoted 15 lines · On Friday, September 23, 2011 16:41:50 Simon Glass wrote:

Quoted 13 lines · Mike Frysinger

Quoted 8 lines · On Friday, September 23, 2011 13:38:51 Simon Glass wrote:

common/console.c +0 -0

@@ -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, CONFIG_SYS_PBSIZE, fmt, args);

i think sizeof(printbuffer) would be better. ?same goes for all the other changes here.
-mike

Quoted 2 lines

Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?

in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard -mike

Quoted 2 lines

in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard

OK - is there a reason why boards can redefine this? Many many boards do. It seems like something that should just be given a sensible value.

Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?

Dear Simon Glass,

Quoted 3 lines · In message <CAPnjgZ0-8wU4Hj3pdmfNMWnj4EPUmQ69U_UARaRt5CbqQv0Ofg@mail.gmail.com>…

Yes, indeed. Could we go as far as removing CONFIG_SYS_PBSIZE, and just use a standard value?

If you can find one that fits for all boards? ?Keep in mind that printf() gets used before relocation, when available stack space may be _very_ limited.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "And it should be the law: If you use ?the ?word ?`paradigm' ?without knowing ?what ?the ?dictionary ?says ?it ?means, ?you ?go to jail. No exceptions." ? ? ? ? ? ? ? ? ? ? - David Jones @ Megatest Corporation

Quoted 4 lines

If you can find one that fits for all boards? =A0Keep in mind that printf() gets used before relocation, when available stack space may be _very_ limited.

Yes that is a problem. Perhaps we could changes things so that this CONFIG really only matters prior to relocation (see other thread with Albert) and we could just choose a suitable small value like 256, which seems to be the minimum for most boards.

Dear Simon Glass,

Quoted 5 lines · In message <CAPnjgZ1Q89Ew2Be2QcjoWF3Nyb+sJ-SxfFunZturMwdLxBc_Wg@mail.gmail.com>…

Quoted 4 lines

If you can find one that fits for all boards? =A0Keep in mind that printf() gets used before relocation, when available stack space may be _very_ limited.

Quoted 4 lines

Yes that is a problem. Perhaps we could changes things so that this CONFIG really only matters prior to relocation (see other thread with Albert) and we could just choose a suitable small value like 256, which seems to be the minimum for most boards.

You are probably addressing a non-issue. ?How many related bug reports can you find in the last decade or so?

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de All your people must learn before you can reach for the stars. ? ? ? ?-- Kirk, "The Gamesters of Triskelion", stardate 3259.2

This tidies up network code to use snprintf() in most cases instead of sprintf(). A few functions remain as they require header file changes.

Quoted 2 lines · On Friday, September 23, 2011 13:38:52 Simon Glass wrote:

This tidies up network code to use snprintf() in most cases instead of sprintf(). A few functions remain as they require header file changes.

NAK to most of these. ?we pick local sized buffers that are known to not overflow, or require circumstances that aren't really feasible.

3 examples (which are the first 3 changes in this patch) below ...

Quoted 3 lines

net/eth.c +0 -0

Quoted 3 lines

? ? ? char buf[20];
- ? ? sprintf(buf, "%pM", enetaddr);
+ ? ? snprintf(buf, sizeof(buf), "%pM", enetaddr);

a mac address will not take more than 19 bytes. ?unless the sprintf code is completely busted, but if that's the case, we should fix that instead since it'd be pretty fundamentally screwed.

Quoted 5 lines

? ? ? char enetvar[32];
- ? ? sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
+ ? ? snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
+ ? ? ? ? ? ? ?base_name, index);

in order for this to overflow, we have to have 1000+ eth devices (maybe more? i'd have to read the code closer)

Quoted 5 lines

? ? ? char enetvar[15];
- ? ? sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
+ ? ? snprintf(enetvar, sizeof(enetvar),
+ ? ? ? ? ? ? index ? "eth%dmacskip" : "ethmacskip", index);

in order for this to overflow, we have to have 10000+ eth devices

please look at the realistic needs rather than blanket converting to snprintf

-mike

Quoted 11 lines · Mike Frysinger

net/eth.c int eth_setenv_enetaddr(char *name, const uchar *enetaddr) · line 52

Quoted 6 lines · On Friday, September 23, 2011 13:38:52 Simon Glass wrote:

net/eth.c +1 -1

 
      char buf[20];
-     sprintf(buf, "%pM", enetaddr);
+     snprintf(buf, sizeof(buf), "%pM", enetaddr);

a mac address will not take more than 19 bytes. unless the sprintf code is completely busted, but if that's the case, we should fix that instead since it'd be pretty fundamentally screwed.

OK (18 bytes?)

Quoted 7 lines

net/eth.c int eth_getenv_enetaddr_by_index(const char *base_name, int index, · line 63

Quoted 4 lines

      char enetvar[32];

net/eth.c +2 -1

-     sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
+     snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
+              base_name, index);

in order for this to overflow, we have to have 1000+ eth devices (maybe more? i'd have to read the code closer)

Or base_name could be longer.

Quoted 7 lines

net/eth.c static int eth_mac_skip(int index) · line 75

Quoted 4 lines

      char enetvar[15];

net/eth.c +2 -1

-     sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
+     snprintf(enetvar, sizeof(enetvar),
+             index ? "eth%dmacskip" : "ethmacskip", index);

in order for this to overflow, we have to have 10000+ eth devices

Quoted 2 lines

please look at the realistic needs rather than blanket converting to snprintf

OK, this is fair enough. There are a couple of functions like ip_to_string() which don't get passed a length. I would prefer to see this done, so we can use snprintf() or assert() since the code then becomes more robust. But there are no bugs there at present. Thoughts?

Quoted 13 lines · On Friday, September 23, 2011 14:30:09 Simon Glass wrote:

Quoted 11 lines · Mike Frysinger

Quoted 6 lines · On Friday, September 23, 2011 13:38:52 Simon Glass wrote:

net/eth.c +0 -0

? ? ? char buf[20];
- ? ? sprintf(buf, "%pM", enetaddr);
+ ? ? snprintf(buf, sizeof(buf), "%pM", enetaddr);

a mac address will not take more than 19 bytes. ?unless the sprintf code is completely busted, but if that's the case, we should fix that instead since it'd be pretty fundamentally screwed.

Quoted 1 line

OK (18 bytes?)

right ... i meant there's 19 available (since 20 makes NUL), and there's no way it should hit that 19 limit.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

? ? ? char enetvar[32];
- ? ? sprintf(enetvar, index ? "%s%daddr" : "%saddr", base_name, index);
+ ? ? snprintf(enetvar, sizeof(enetvar), index ? "%s%daddr" : "%saddr",
+ ? ? ? ? ? ? ?base_name, index);

Quoted 4 lines

Quoted 2 lines

in order for this to overflow, we have to have 1000+ eth devices (maybe more? i'd have to read the code closer)

Or base_name could be longer.

true, but base_name atm is supposed to be "eth" or "usbeth". ?i would hope we'd be diligent about device naming conventions rather than letting someone slip in "mysuperawesomeboardeth" (although that still wouldn't overflow ;]).

Quoted 8 lines

Quoted 8 lines

Quoted 5 lines

? ? ? char enetvar[15];
- ? ? sprintf(enetvar, index ? "eth%dmacskip" : "ethmacskip", index);
+ ? ? snprintf(enetvar, sizeof(enetvar),
+ ? ? ? ? ? ? index ? "eth%dmacskip" : "ethmacskip", index);

in order for this to overflow, we have to have 10000+ eth devices

Quoted 7 lines

Quoted 2 lines

please look at the realistic needs rather than blanket converting to snprintf

OK, this is fair enough. There are a couple of functions like ip_to_string() which don't get passed a length. I would prefer to see this done, so we can use snprintf() or assert() since the code then becomes more robust. But there are no bugs there at present. Thoughts?

for funcs that do generally handle arbitrary data, i don't mind including len specs, but when we're using known standards, i'd prefer to stick to the smaller/simpler code. ?this is a thin boot loader that is meant to be fast, and there are plenty of ways to bring down the system otherwise (and i don't think these cases are generally a robustness concern).

don't know if wolfgang has an opinion.

-mike

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.

Hi Simon,

Le 23/09/2011 19:38, Simon Glass a ?crit :

Quoted 7 lines

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.

Indeed overruns can lead to bad behaviors, but in any case, it can never be recovered, because at the root, the problem is that the caller provided inconsistent arguments to printf.

So in essence, you're 'fixing' printf for a design error in printf's caller, instead of fixing the design error.

Amicalement,
--
Albert.

Hi Albert,

Quoted 11 lines · Albert ARIBAUD

Hi Simon,

Le 23/09/2011 19:38, Simon Glass a ?crit :

Quoted 7 lines

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.

Quoted 3 lines

Indeed overruns can lead to bad behaviors, but in any case, it can never be recovered, because at the root, the problem is that the caller provided inconsistent arguments to printf.

Recovery is one thing, but I would settle for just not crashing, which is the purpose of this patch. We could also easily WARN if that were considered appropriate here.

Quoted 1 line

Quoted 2 lines

So in essence, you're 'fixing' printf for a design error in printf's caller, instead of fixing the design error.

Well, the nature of a function is that it cannot know what arguments might be passed to it. It can only assert(), limit check, etc. A limit check is what this patch aims to add.

Regards,
Simon

Le 23/09/2011 22:46, Simon Glass a ?crit :

Quoted 28 lines

Hi Albert,

Quoted 13 lines · On Fri, Sep 23, 2011 at 1:40 PM, Albert ARIBAUD <[email protected]>…

Hi Simon,

Le 23/09/2011 19:38, Simon Glass a ?crit :

Quoted 8 lines

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.

Quoted 4 lines

Indeed overruns can lead to bad behaviors, but in any case, it can never be
recovered, because at the root, the problem is that the caller provided inconsistent arguments to printf.

Recovery is one thing, but I would settle for just not crashing, which is the purpose of this patch. We could also easily WARN if that were considered appropriate here.

Quoted 1 line

Quoted 7 lines

Quoted 3 lines

So in essence, you're 'fixing' printf for a design error in printf's caller,
instead of fixing the design error.

Well, the nature of a function is that it cannot know what arguments might be passed to it. It can only assert(), limit check, etc. A limit check is what this patch aims to add.

I do agree that as a rule, a function should check its arguments, but as any rule, this one has understated assumptions. Here, one assumption is that calls to the functions cannot be made compliant, and therefore it falls to the function to ensure this compliance; and another one is that the function can actually check this conformance.

The first assumption is correct in an OS or general-purpose library where the number of callers is virtually unlimited and there is no way to make sure all calls are conforming.

In U-Boot however, the number of callers is bound and known (unless you're thinking of standalone U-Boot apps, but I don't see this as a use case so frequent that it warrants a 'fix')

The second assumption is false by nature for printf(), which simply cannot check its input buffer (OTOH *nprintf() does, and it is its job).

Besides, there *is* a sub-family of printf functions which are dedicated to the case where buffers provided may be too small for the print output -- meaning that the issue is known and an easy fix exists if cropping the output is allowable.

So basically the choice is between:

- adding code to the printf() family to try and fix an issue that it is fundamentally unable to properly fix, and for which a solution exists, or

- grepping and fixing calls to *sprintf() in U-Boot that do not respect the known contraints of printf(), by resizing the buffer or calling *snprintf() instead.

I am definitely not in favor of the first option concerning U-Boot.

Amicalement,
--
Albert.

Quoted 9 lines

So basically the choice is between:

- adding code to the printf() family to try and fix an issue that it is fundamentally unable to properly fix, and for which a solution exists, or

- grepping and fixing calls to *sprintf() in U-Boot that do not respect the known contraints of printf(), by resizing the buffer or calling *snprintf() instead.

Quoted 1 line

I am definitely not in favor of the first option concerning U-Boot.

Sounds fine to me. So I think we need the nprintf() variants in there, but the message is not to use them willy nilly. Going back to my patch series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that right?

By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.

Regards,
Simon

Le 24/09/2011 16:00, Simon Glass a ?crit :

Quoted 11 lines

Quoted 11 lines

So basically the choice is between:

- adding code to the printf() family to try and fix an issue that it is fundamentally unable to properly fix, and for which a solution exists, or

- grepping and fixing calls to *sprintf() in U-Boot that do not respect the
known contraints of printf(), by resizing the buffer or calling *snprintf()
instead.

Quoted 6 lines

Quoted 1 line

I am definitely not in favor of the first option concerning U-Boot.

Sounds fine to me. So I think we need the nprintf() variants in there, but the message is not to use them willy nilly. Going back to my patch series, 3/4 is ok, but 4/4 mostly crosses the line. Do I have that right?

It is the exact opposite for me : 3/4 makes all printf functions work like some kind of *nprintf(), while 4/4 is about the network code switching to *nprintf() for safety, so 3/4 would be nak and 4/4 ack as far as I am concerned.

Basically, printf family functions which do not have the 'n' are *know* by all -- experienced enough :) -- programmers to be *unsafe* (but to require less from the caller) and it should remain so: no programmer should ever encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on another C library where printf is just the beartrap it usually is.

IOW, programmers already have assumptions about *printf(), including how to deal with length limitations and what happens if you don't, and it is best that these assumption remain true whatever project they work with.

Quoted 5 lines

By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.

I don't intend to dictate the way things can be implemented, so the degree of code reuse is an open question as far as I am concerned. I am only voicing my opinion that *printf() APIs and their contracts should remain identical across all implementations of *printf(), and thus that providing *nprintf() where they don't exist is commandable, but hardening printf() is not, since you basically cannot do it without somewhat departing from the de facto standard.

Quoted 3 lines

Regards,
Simon

Amicalement,
--
Albert.

Quoted 10 lines

Basically, printf family functions which do not have the 'n' are *know* by all -- experienced enough :) -- programmers to be *unsafe* (but to require less from the caller) and it should remain so: no programmer should ever encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on another C library where printf is just the beartrap it usually is.

IOW, programmers already have assumptions about *printf(), including how to deal with length limitations and what happens if you don't, and it is best that these assumption remain true whatever project they work with.

I don't quite understand this. You are saying that we shouldn't modify the existing printf(), serial_printf() etc. so live within the buffer they use? If I call printf() and my resulting string is too long for the library then I would much prefer to get truncated output than have to hunt for a wierd crash.

It sounds like you are asking for a new printf(), serial_printf() which provides the facility of limiting the output to n characters, and leave these ones alone. But these functions are not passed a buffer - they use their own and they set the site of it. So I think they should be responsible for enforcing that size.

If you are asking for a new set of functions - nprintf(), serial_nprintf(), etc. then I wonder how you can pass the 'n' but not the actual buffer. In my mind you should not do one without the other.

So please can you clarify?

Quoted 5 lines

Quoted 4 lines

By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.

Quoted 7 lines

I don't intend to dictate the way things can be implemented, so the degree of code reuse is an open question as far as I am concerned. I am only voicing my opinion that *printf() APIs and their contracts should remain identical across all implementations of *printf(), and thus that providing *nprintf() where they don't exist is commandable, but hardening printf() is not, since you basically cannot do it without somewhat departing from the de facto standard.

OK fair enough. People are used to printf() just working, no matter what the size. I haven't looked at the implementation but I suspect when the buffer fills it outputs it and starts the buffer again .In any case you don't have to worry about it with the GNU C library.

We probably don't need to go that far in U-Boot, but some simple checking would avoid a nasty surprise for the user. It is obvious from the result that something is truncated, and we can WARN if that helps.

Regards,
Simon

Hi Simon,

Le 25/09/2011 16:50, Simon Glass a ?crit :

Quoted 16 lines

Quoted 10 lines

Basically, printf family functions which do not have the 'n' are *know* by
all -- experienced enough :) -- programmers to be *unsafe* (but to require
less from the caller) and it should remain so: no programmer should ever encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on
another C library where printf is just the beartrap it usually is.

Quoted 5 lines

IOW, programmers already have assumptions about *printf(), including how to
deal with length limitations and what happens if you don't, and it is best
that these assumption remain true whatever project they work with.

Quoted 5 lines

I don't quite understand this. You are saying that we shouldn't modify the existing printf(), serial_printf() etc. so live within the buffer they use? If I call printf() and my resulting string is too long for the library then I would much prefer to get truncated output than have to hunt for a wierd crash.

I understand the preference for truncated output, but the output length is primarily in the hands of the caller, through the format stringand possibly also through *nprintf() -- i.e. the caller can only get an output overrun if (s)he does not take the required steps to avoid it.

Quoted 6 lines

It sounds like you are asking for a new printf(), serial_printf() which provides the facility of limiting the output to n characters, and leave these ones alone. But these functions are not passed a buffer - they use their own and they set the site of it. So I think they should be responsible for enforcing that size.

The existing contract for printf family functions (think wider than U-Boot) does not enforce this responsibility on them.

Quoted 6 lines

If you are asking for a new set of functions - nprintf(), serial_nprintf(), etc. then I wonder how you can pass the 'n' but not the actual buffer. In my mind you should not do one without the other.

So please can you clarify?

I should clarify indeed. My opinion, expressed as a single general rule, is "keep the known semantics of *printf() function family unchanged in U-Boot wrt all other printif implementations around". Thus I think that:

- the printf() function should *not* attempt anything to mitigate length issues beyond what the standard mandates. If a user calls printf(), (s)he is expected to be aware of the risks of overruns, ?and to take -- if (s)he so decides -- steps to avoid it; besides, the function does not have a way. Conversively, implementers of the printf() function need not consider any specific recovery action with regard to size issues. For instance, why do we have an internal buffer for printf to begin with? The implementation does not need them (and besides, I guess the buffer does not respect the single C99 environmental constraint for printf, that any field should be able to be at least 4095 bytes -- no kidding).

- users who actually wisht to limit outpout ca use either

-- a crafted format string with maximum-sized format specifiers, or

-- a call tp a *nprintf() function into a local buffer of known size followed by a call to fputs().

Quoted 18 lines

Quoted 6 lines

Quoted 5 lines

By the way, printf() ends up calling the same code, but without limit checking in place. The alternative is to duplicate all the format string processing code (a limit-checking version and an unchecked version) which would be worse.

Quoted 11 lines

I don't intend to dictate the way things can be implemented, so the degree
of code reuse is an open question as far as I am concerned. I am only voicing my opinion that *printf() APIs and their contracts should remain identical across all implementations of *printf(), and thus that providing
*nprintf() where they don't exist is commandable, but hardening printf() is
not, since you basically cannot do it without somewhat departing from the de
facto standard.

Quoted 4 lines

OK fair enough. People are used to printf() just working, no matter what the size. I haven't looked at the implementation but I suspect when the buffer fills it outputs it and starts the buffer again .In any case you don't have to worry about it with the GNU C library.

You may also find implementations where the buffer is flushed after each literal part in the format string and after each format specifier output. Or some which emit serial characters as soon as they are produced and use a buffer only for those formats where the ASCII representation cannot be easily constructed left-to-right.

Quoted 3 lines

We probably don't need to go that far in U-Boot, but some simple checking would avoid a nasty surprise for the user. It is obvious from the result that something is truncated, and we can WARN if that helps.

A user who does not expect a nasty surprise from calling printf() with a fair chance of overflowing the output buffer deserves the surprise. :)

Quoted 3 lines

Regards,
Simon

Amicalement,
--
Albert.

For sprintf() I agree - this is well understood and people are aware of it. For printf() I am not so sure.

Hi Simon,

Le 25/09/2011 16:50, Simon Glass a ?crit :

Quoted 3 lines

Quoted 3 lines

Basically, printf family functions which do not have the 'n' are *know* by all -- experienced enough :) -- programmers to be *unsafe* (but to require less from the caller)

and it should remain so: no programmer should ever

Quoted 3 lines

Quoted 3 lines

encounter an implementation of printf that pretends to be even somewhat safe, because it might bite him/her elsewhere, in another project based on another C library where printf is just the beartrap it usually is.

I should clarify indeed. My opinion, expressed as a single general rule, is "keep the known semantics of *printf() function family unchanged in U-Boot wrt all other printif implementations around". Thus I think that:

Conversively, implementers of the printf() function need not consider any specific recovery action with regard to size issues. For instance, why do we have an internal buffer for printf to begin with? The implementation does not need them

(and besides, I guess the buffer
does not respect the single C99 environmental constraint for printf, that any field should be able to be at least 4095 bytes -- no kidding).

- users who actually wisht to limit outpout ca use either

Quoted 1 line

- users who actually wisht to limit outpout ca use either

You say "actually wish to limit output" as if "let it corrupt memory if it's too large" is the normal thing to want.

-scott

Quoted 4 lines · Scott Wood

Quoted 1 line

- users who actually wisht to limit outpout ca use either

You say "actually wish to limit output" as if "let it corrupt memory if it's too large" is the normal thing to want.

What I meant was "users who actually want to limit output explicitly by truncating it rather than implicitly by always outputting small enough text or crafting limitations in the format string specifiers, ..."

Quoted 1 line

-scott

Amicalement,
--
Albert.

Just to follow up this thread, I am going to drop the network patch and resubmit the rest of it. This will make snprintf() available in U-Boot.

Dear Simon Glass,

Quoted 4 lines · In message <CAPnjgZ3hYmXEHr8MT0deZ054kuOxkNDt4Sa+nVPSzPAdAQFf0w@mail.gmail.com>…

Just to follow up this thread, I am going to drop the network patch and resubmit the rest of it. This will make snprintf() available in U-Boot.

Without code that uses it? ?We don't accept dead code...

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Due to lack of disk space, this fortune database has been discontinued.

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, and also some network code (as an example of where this might lead).

Dear Simon Glass,

Quoted 5 lines · In message <[email protected]> you wrote:

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.

Quoted 2 lines

This patch series tidies this up in the common vsprintf.c code, and also some network code (as an example of where this might lead).

What's the impact of this patch set on the memory footprint of typical configurations?

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de ? ? ? ? ? ? ? ? ?Nail here --X-- for new monitor.