From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rae Date: Thu, 7 Aug 2014 09:52:44 -0700 Subject: [U-Boot] [PATCH v3 2/4] usb/gadget: fastboot: add eMMC support for flash command In-Reply-To: <201408071523.42634.marex@denx.de> References: <1403813604-31685-1-git-send-email-srae@broadcom.com> <201408070213.35459.marex@denx.de> <53E2C81D.6050003@broadcom.com> <201408071523.42634.marex@denx.de> Message-ID: <53E3AEDC.5080305@broadcom.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 14-08-07 06:23 AM, Marek Vasut wrote: > On Thursday, August 07, 2014 at 02:28:13 AM, Steve Rae wrote: >> On 14-08-06 05:13 PM, Marek Vasut wrote: >>> On Thursday, August 07, 2014 at 01:48:06 AM, Steve Rae wrote: >>>> On 14-07-30 06:37 PM, Marek Vasut wrote: >>>>> On Thursday, June 26, 2014 at 10:13:22 PM, Steve Rae wrote: >>>>> [...] >>>>> >>>>>> + >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> +#include >>>>>> + >>>>>> +/* The 64 defined bytes plus \0 */ >>>>>> +#define RESPONSE_LEN (64 + 1) >>>>>> + >>>>>> +static char *response_str; >>>>> >>>>> I'd suggest to pass this "response_str" around instead of making it >>>>> global. >>>> >>>> That would involve adding it to fastboot_resp(), which is called 11 >>>> times in this code, from 3 different functions (so would need to add >>>> this to two of the functions...). And as these evolve, there will likely >>>> be more nested functions, which would all require "passing it >>>> around".... I think that this "static global pointer" is a cleaner >>>> implementation. >>> >>> Eventually, the amount of these static variables in the code will grow >>> and it will become increasingly difficult to weed them out. I believe it >>> would be even better to pass around some kind of a structure with >>> "private data" of the fastboot, which would cater for all possible >>> variables which might come in the future. What do you think ? >> >> Yes -- if there is private data that the fastboot implementation >> requires, then a data structure is the way to go. However, I still think >> that this "fastboot response string" would even be an exception to that >> private data.... > > OK, let's leave it this way for now. > >>>>>> +static void fastboot_resp(const char *s) >>>>>> +{ >>>>>> + strncpy(response_str, s, RESPONSE_LEN); >>>>>> + response_str[RESPONSE_LEN - 1] = '\0'; >>>>> >>>>> This could be shrunk to a single snprintf(response_str, >>>>> RESPONSE_LENGTH, s); I think, but I'm not sure if the overhead won't >>>>> grow. >>>> >>>> snprintf() is used very sparingling in U-Boot >>> >>> This is not a reason to avoid it. >> >> true.... >> >>>> , and with the cautionary statements in README (line 852) >>> >>> Which statements? Can you please point them out? I fail to see them, >>> sorry. >> >> I was referring to what you mention below... >> 852 - Safe printf() functions >> 853 Define CONFIG_SYS_VSNPRINTF to compile in safe versions of >> 854 the printf() functions. These are defined in >> 855 include/vsprintf.h and include snprintf(), vsnprintf() and >> 856 so on. Code size increase is approximately 300-500 bytes. >> 857 If this option is not given then these functions will >> 858 silently discard their buffer size argument - this means >> 859 you are not getting any overflow checking in this case. > > I really don't see the "cautionary statements" here , no . I see that it > discards the size checking if this CONFIG_SYS_VSNPRINTF is not enabled, but that > does not obstruct the operation of those functions. > I'm really confused: my code ensures that the buffer is not overflowed and that it is terminated properly. If snprintf() (without CONFIG_SYS_VSNPRINTF defined) doesn't provide "any overflow checking", then why would I use it? >>>> and the fact that CONFIG_SYS_VSNPRINTF is not defined for armv7 builds, >>>> I am >>> >>> not going to use it.... >>> >>> Is it a problem to define it? Also, even without CONFIG_SYS_VSNPRINTF , >>> the >>> >>> functions are still available, see the README: >>> 857 If this option is not given then these functions >>> will 858 silently discard their buffer size argument - >>> this means 859 you are not getting any overflow >>> checking in this case. >>> >>> I have yet to see some hard-evidence against using safe printing >>> functions here. >> >> I don't want to be the first to defined it for all of armv7.... > > Honestly, we should just enable this CONFIG_SYS_VSNPRINTF by default for the > good of humanity and all the things, since this unbounded string handling is > just evil (see how OpenSSL ended up, partly because of that ... and I am just > starting to see the pattern in all the security code). I don't want to go down > that road with U-Boot. > > So, would you please cook a separate patch to enable this by default, so it > would spur the right kind of discussion on this matter ? I will apologize in advance, but I just don't know anything about SPL or TPL or any other boards (outside of my very limited armv7 and armv8 scope).... I would be happy to review and test this suggested patch (on our boards), but would be uncomfortable with proposing this patch. Please go ahead and submit a patch, and I'll check it! Thanks, Steve > >> And I really don't want to define it only only my boards running so that >> they can run 'fastboot' >> What do you suggest? > > See above, thanks ! >