| Message ID | 1316799532-20761-4-git-send-email-sjg@chromium.org |
|---|---|
| State | New, archived |
| Headers | show |
On Sep 23, 2011, at 12:38 PM, Simon Glass wrote: > 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: Simon Glass <sjg@chromium.org> > --- > 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
Hi Kumar, On Fri, Sep 23, 2011 at 11:36 AM, Kumar Gala <galak@kernel.crashing.org> wrote: > > On Sep 23, 2011, at 12:38 PM, Simon Glass wrote: > >> 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: Simon Glass <sjg@chromium.org> >> --- >> 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. > OK will fix that with next version thanks. The commit in my tree says Sonny but perhaps git format-patch --signoff is not doing what I expect. Regards, Simon > - k >
On Friday, September 23, 2011 13:38:51 Simon Glass wrote: > --- 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, CONFIG_SYS_PBSIZE, fmt, args); i think sizeof(printbuffer) would be better. same goes for all the other changes here. -mike
Hi Mike, On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Friday, September 23, 2011 13:38:51 Simon Glass wrote: >> --- 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, 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? Regards, Simon
On Friday, September 23, 2011 16:41:50 Simon Glass wrote: > On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger wrote: > > On Friday, September 23, 2011 13:38:51 Simon Glass wrote: > >> --- 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, 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? in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard -mike
Hi Mike, On Fri, Sep 23, 2011 at 3:36 PM, Mike Frysinger <vapier@gentoo.org> wrote: > On Friday, September 23, 2011 16:41:50 Simon Glass wrote: >> On Fri, Sep 23, 2011 at 1:31 PM, Mike Frysinger wrote: >> > On Friday, September 23, 2011 13:38:51 Simon Glass wrote: >> >> --- 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, 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? > > in the context of I/O funcs, CONFIG_SYS_PBSIZE is the current standard > -mike > 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. Regards, Simon
Dear Simon Glass, In message <CAPnjgZ0-8wU4Hj3pdmfNMWnj4EPUmQ69U_UARaRt5CbqQv0Ofg@mail.gmail.com> you wrote: > > 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
Dear Simon Glass, In message <CAPnjgZ2D8MYS7wHMFUnzVhXK_tcmv7GayJyEormkPDNA78p7JQ@mail.gmail.com> you wrote: > > > 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. Indeed - that's exactly what the board specific definitions are supposed to do. Best regards, Wolfgang Denk
HI Wolfgang, On Sun, Sep 25, 2011 at 1:14 PM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon Glass, > > In message <CAPnjgZ0-8wU4Hj3pdmfNMWnj4EPUmQ69U_UARaRt5CbqQv0Ofg@mail.gmail.com> you wrote: >> >> 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. 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. Regards, Simon > > 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@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 >
Dear Simon Glass, In message <CAPnjgZ1Q89Ew2Be2QcjoWF3Nyb+sJ-SxfFunZturMwdLxBc_Wg@mail.gmail.com> you wrote: > > > 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. 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
Hi Wolfgang, On Mon, Sep 26, 2011 at 11:47 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear Simon Glass, > > In message <CAPnjgZ1Q89Ew2Be2QcjoWF3Nyb+sJ-SxfFunZturMwdLxBc_Wg@mail.gmail.com> you wrote: >> >> > 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. > > You are probably addressing a non-issue. How many related bug reports > can you find in the last decade or so? > Yes probably, it isn't important and is off-topic from my patch anyway. Regards, Simon > 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@denx.de > All your people must learn before you can reach for the stars. > -- Kirk, "The Gamesters of Triskelion", stardate 3259.2 >
diff --git a/common/console.c b/common/console.c index 8c650e0..6057e9a 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, CONFIG_SYS_PBSIZE, 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, CONFIG_SYS_PBSIZE, fmt, args); va_end(args); /* Send to desired file */ @@ -376,7 +376,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, CONFIG_SYS_PBSIZE, fmt, args); va_end(args); /* Print the string */ @@ -392,7 +392,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, CONFIG_SYS_PBSIZE, fmt, args); /* Print the string */ puts(printbuffer); @@ -459,7 +459,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, CONFIG_SYS_PBSIZE, fmt, args); va_end(args); if ((screen + sizeof(screen) - 1 - cursor)