On 07/18/2013 09:25 AM, Jeff Layton wrote: > On Thu, 18 Jul 2013 09:04:30 +0800 > Chen Gang <gang.chen@xxxxxxxxxxx> wrote: > >> > On 07/17/2013 07:24 PM, Jeff Layton wrote: >>> > > On Tue, 16 Jul 2013 22:47:35 -0500 >>> > > Shirish Pargaonkar <shirishpargaonkar@xxxxxxxxx> wrote: >>> > > >>>> > >> nitpicking... >>>> > >> >>>> > >> Should it be MAX_CIFS_DOMAINNAME instead of MAX_CIF_DOMAINNAME, >>>> > >> unless CIF is short for something here? >>>> > >> >>>> > >> Regards, >>>> > >> >>>> > >> Shirish >>>> > >> >>> > > >>> > > Even better... >>> > > >>> > > We already have a MAX_USERNAME_SIZE. Maybe call it MAX_DOMAINNAME_SIZE >>> > > for parity with that? Might also want to relocate the #define next to >>> > > that one since it would be helpful to have all of those lengths grouped >>> > > in the same place. >>> > > >> > >> > It sounds reasonable: use MAX_DOMAINNAME_SIZE instead of >> > MAX_CIFS_DOMAINNAME. >> > >> > >>>> > >> On Tue, Jul 16, 2013 at 7:48 PM, Chen Gang <gang.chen@xxxxxxxxxxx> wrote: >>>>> > >>> For cifs_set_cifscreds() in "fs/cifs/connect.c", 'desc' buffer length >>>>> > >>> is 'CIFSCREDS_DESC_SIZE' (56 is less than 256), and 'ses->domainName' >>>>> > >>> length may be "255 + '\0'". >>>>> > >>> >>>>> > >>> The related sprintf() may cause memory overflow, so need extend related >>>>> > >>> buffer enough to hold all things. >>>>> > >>> >>> > > >>> > > Good catch... >>> > > >>> > > Maybe it would be good to go ahead and turn that sprintf() into a >>> > > snprintf() too? >>> > > >> > >> > Hmm... sprintf() declares to code readers, in current condition, we want >> > to print all source information without any truncation. >> > >> > So if we know the source max length precisely, we'd better to allocate >> > the related buffer to print them all instead of use snprintf(). >> > >> > If we do not know the source max length precisely or we have to limit >> > the destination length, we need use snprintf() to fit with destination >> > max length (declare to the code readers, the source information may be >> > truncated). >> > >> > > Fair enough. It was more of a suggestion for defensive coding. But > since we know the length of both buffers and that the source is NULL > terminated, then sprintf is fine. > > Patch looks fine to me otherwise. > > Acked-by: Jeff Layton <jlayton@xxxxxxxxxx> > > Thank you for your Acked-by. If possible, I will/should wait a day, if no another additional suggestions or completions, I should send patch v2 tomorrow. Thanks. -- Chen Gang -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
