[PATCH] cifs: extend the buffer length enought for sprintf() using
Chen Gang
gang.chen at asianux.com
Wed Jul 17 19:31:51 MDT 2013
On 07/18/2013 09:25 AM, Jeff Layton wrote:
> On Thu, 18 Jul 2013 09:04:30 +0800
> Chen Gang <gang.chen at asianux.com> wrote:
>
>> > On 07/17/2013 07:24 PM, Jeff Layton wrote:
>>> > > On Tue, 16 Jul 2013 22:47:35 -0500
>>> > > Shirish Pargaonkar <shirishpargaonkar at gmail.com> 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 at asianux.com> 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 at redhat.com>
>
>
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
More information about the samba-technical
mailing list