Btrfs devel - Feb 2011 - [PATCH] Btrfs-progs use safe string manipulation functions

Please find the attached patch which replace unsafe strcpy(3) by
strncpy(3) functions.

regards, 

Eduardo Silva

On 02/07/2011 01:22 PM, Eduardo Silva wrote:
> Please find the attached patch which replace unsafe strcpy(3) by
> strncpy(3) functions.
> 
> regards, 
> 
> Eduardo Silva

Hi Eduardo,

even though some "strncpy" are unneeded because a check is performed
before, I fully agree that "strncpy" is better than a simple strcpy.
However I have to highlight that truncating a path without noticing may
be lead to a problem (think a delete operation applied to a shorter
path...).
So can I ask you to add some checks to your patch, in order to warn the
user that the path is too long ?

Regards
G.Baroncelli
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2011-02-07 13:22, Eduardo Silva wrote:
> Please find the attached patch which replace unsafe strcpy(3) by
> strncpy(3) functions.
strncpy() doesn''t NUL-terminate the destination buffer if the
maximum length is reached.  And as far as I can see, there is
no other initialization of those buffers to zeroes, except for
super.label in make_btrfs() in utils.c.

So please change those strncpy() calls to something like:

    strncpy(args.name, source, BTRFS_PATH_NAME_MAX);
    args.name[BTRFS_PATH_NAME_MAX] = ''\0'';

(Note that the name member of struct btrfs_ioctl_vol_args is
BTRFS_PATH_NAME_MAX + 1 long, so the above is correct for that
field.)

And of course similarly in those cases where you copy to something
other than a struct btrfs_ioctl_vol_args.


There were also a two places where you used spaces instead of tabs
for indentation (in main() in btrfsctl.c, and the declaration of
pretty_len in pretty_sizes() in utils.c).


	/Bellman
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman <bellman@nsc.liu.se>
wrote:
>    strncpy(args.name, source, BTRFS_PATH_NAME_MAX);
>    args.name[BTRFS_PATH_NAME_MAX] = ''\0'';
That''s silly. Isn''t there a sane safe variant of strcpy?

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Olaf van der Spek wrote:
> On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman <bellman@nsc.liu.se>
> wrote:
>> strncpy(args.name, source, BTRFS_PATH_NAME_MAX);
>> args.name[BTRFS_PATH_NAME_MAX] = ''\0'';
> 
> That''s silly. Isn''t there a sane safe variant of strcpy?
There''s strlcpy, but it''s not in glibc because of possible
truncation
errors!

http://en.wikipedia.org/wiki/Strlcpy

Of course C++ strings would be much better... :-)

Jeremy


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Feb 10, 2011 at 12:37 PM, Jeremy Sanders
<jeremy@jeremysanders.net> wrote:
> Olaf van der Spek wrote:
>
>> On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman
<bellman@nsc.liu.se>
>> wrote:
>>> strncpy(args.name, source, BTRFS_PATH_NAME_MAX);
>>> args.name[BTRFS_PATH_NAME_MAX] = ''\0'';
>>
>> That''s silly. Isn''t there a sane safe variant of
strcpy?
>
> There''s strlcpy, but it''s not in glibc because of
possible truncation
> errors!
Then use a private wrapper.

-- 
Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2011-02-10 at 12:08 +0100, Thomas Bellman wrote:
> On 2011-02-07 13:22, Eduardo Silva wrote:
> 
> > Please find the attached patch which replace unsafe strcpy(3) by
> > strncpy(3) functions.
> 
> strncpy() doesn''t NUL-terminate the destination buffer if the
> maximum length is reached.  And as far as I can see, there is
> no other initialization of those buffers to zeroes, except for
> super.label in make_btrfs() in utils.c.
> 
> So please change those strncpy() calls to something like:
> 
>     strncpy(args.name, source, BTRFS_PATH_NAME_MAX);
>     args.name[BTRFS_PATH_NAME_MAX] = ''\0'';
> 
Seems like a string manipulation function is the way to go, will send a
new patch shortly, 

best, 

Eduardo

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On to, 2011-02-10 at 11:37 +0000, Jeremy Sanders wrote:
> There''s strlcpy, but it''s not in glibc because of
possible truncation
> errors!
snprintf is standard, and should be about as safe as it gets with the
glibc functions.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Feb 10, 2011 at 12:54 PM, Lars Wirzenius <liw@liw.fi>
wrote:
> On to, 2011-02-10 at 11:37 +0000, Jeremy Sanders wrote:
>> There''s strlcpy, but it''s not in glibc because of
possible truncation
>> errors!
>
> snprintf is standard, and should be about as safe as it gets with the
> glibc functions.
But snprintf is not like strlcpy.

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 2011-02-10 13:27, Olaf van der Spek wrote:
> On Thu, Feb 10, 2011 at 12:54 PM, Lars Wirzenius <liw@liw.fi> wrote:
>> snprintf is standard, and should be about as safe as it gets with the
>> glibc functions.
> 
> But snprintf is not like strlcpy.
It is indeed uglier to write ''snprintf(dst, size, "%s",
src)'' instead of
''strlcpy(dst, src, size)'', but both the effect and the return
value should
be identical.

	/Bellman
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2011-02-10 at 12:39 +0100, Olaf van der Spek
wrote:
> On Thu, Feb 10, 2011 at 12:37 PM, Jeremy Sanders
> <jeremy@jeremysanders.net> wrote:
> > Olaf van der Spek wrote:
> >
> >> On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman
<bellman@nsc.liu.se>
> >> wrote:
> >>> strncpy(args.name, source, BTRFS_PATH_NAME_MAX);
> >>> args.name[BTRFS_PATH_NAME_MAX] = ''\0'';
> >>
> >> That''s silly. Isn''t there a sane safe variant of
strcpy?
> >
> > There''s strlcpy, but it''s not in glibc because of
possible truncation
> > errors!
> 
> Then use a private wrapper.
> 
Here''s the new patch:

----
[PATCH] Add safe string manipulation functions

Deprecate direct use of strcpy(3)
The following string manipulation function has been added:

   - string_copy() : wrapper of strcpy(3)
   - string_ncopy(): wrapper of strncpy(3)

both function compose safe NULL terminated strings.
----

I check that the code most of the time raise an error if the path is too
long, so the new wrappers should be ok...

best, 

Eduardo Silva

On Thu, Feb 10, 2011 at 2:29 PM, Eduardo Silva <eduardo.silva@oracle.com>
wrote:
>> > There''s strlcpy, but it''s not in glibc because
of possible truncation
>> > errors!
>>
>> Then use a private wrapper.
>>
>
> Here''s the new patch:
>
> ----
> [PATCH] Add safe string manipulation functions
>
> Deprecate direct use of strcpy(3)
> The following string manipulation function has been added:
>
>   - string_copy() : wrapper of strcpy(3)
>   - string_ncopy(): wrapper of strncpy(3)
>
> both function compose safe NULL terminated strings.
> ----
>
> I check that the code most of the time raise an error if the path is too
> long, so the new wrappers should be ok...
string_copy seems pointless, it''s kinda equivalent to strcpy.
if (!dest || !src) should include an assert so it''s easier to break in
the debugger.

-- 
Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2011-02-10 at 14:34 +0100, Olaf van der Spek
wrote:
> On Thu, Feb 10, 2011 at 2:29 PM, Eduardo Silva
<eduardo.silva@oracle.com> wrote:
> >> > There''s strlcpy, but it''s not in glibc
because of possible truncation
> >> > errors!
> >>
> >> Then use a private wrapper.
> >>
> >
> > Here''s the new patch:
> >
> > ----
> > [PATCH] Add safe string manipulation functions
> >
> > Deprecate direct use of strcpy(3)
> > The following string manipulation function has been added:
> >
> >   - string_copy() : wrapper of strcpy(3)
> >   - string_ncopy(): wrapper of strncpy(3)
> >
> > both function compose safe NULL terminated strings.
> > ----
> >
> > I check that the code most of the time raise an error if the path is
too
> > long, so the new wrappers should be ok...
> 
> string_copy seems pointless, it''s kinda equivalent to strcpy.
got your point, but If we are creating wrappers for string manipulation,
let''s do it for the most common functions used.
> if (!dest || !src) should include an assert so it''s easier to
break in
> the debugger.
> 
  good one

regards, 

Ed.-

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Feb 10, 2011 at 2:37 PM, Eduardo Silva <eduardo.silva@oracle.com>
wrote:
> string_copy seems pointless, it''s kinda equivalent to strcpy.
>
> Yeah, but if we are thinking into write some wrappers let''s create
a couple
> for the major string manipulation used...
A wrapper should have a benefit, your string_copy doesn''t have any.
-- 
Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2011-02-10 at 14:52 +0100, Olaf van der Spek
wrote:
> On Thu, Feb 10, 2011 at 2:37 PM, Eduardo Silva
<eduardo.silva@oracle.com> wrote:
> > string_copy seems pointless, it''s kinda equivalent to strcpy.
> >
> > Yeah, but if we are thinking into write some wrappers let''s
create a couple
> > for the major string manipulation used...
> 
> A wrapper should have a benefit, your string_copy doesn''t have
any.
It have it, take this example:

	char *a;
        a = malloc(1024);
        strcpy(a, NULL);

at least with the wrapper you will get a notice about what''s going
on... 



--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Feb 10, 2011 at 3:00 PM, Eduardo Silva <eduardo.silva@oracle.com>
wrote:
> On Thu, 2011-02-10 at 14:52 +0100, Olaf van der Spek wrote:
>> On Thu, Feb 10, 2011 at 2:37 PM, Eduardo Silva
<eduardo.silva@oracle.com> wrote:
>> > string_copy seems pointless, it''s kinda equivalent to
strcpy.
>> >
>> > Yeah, but if we are thinking into write some wrappers
let''s create a couple
>> > for the major string manipulation used...
>>
>> A wrapper should have a benefit, your string_copy doesn''t have
any.
>
> It have it, take this example:
>
>        char *a;
>        a = malloc(1024);
>        strcpy(a, NULL);
>
> at least with the wrapper you will get a notice about what''s going
> on...
The debugger shows you what''s going on without wrapper.

-- 
Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, Feb 10, 2011 at 12:37 PM, Jeremy Sanders
<jeremy@jeremysanders.net> wrote:
> Of course C++ strings would be much better... :-)
Yeah, why isn''t C++ being used?

Olaf
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 02/10/2011 02:29 PM, Eduardo Silva wrote:
> On Thu, 2011-02-10 at 12:39 +0100, Olaf van der Spek wrote:
>> On Thu, Feb 10, 2011 at 12:37 PM, Jeremy Sanders
>> <jeremy@jeremysanders.net> wrote:
>>> Olaf van der Spek wrote:
>>>
>>>> On Thu, Feb 10, 2011 at 12:08 PM, Thomas Bellman
<bellman@nsc.liu.se>
>>>> wrote:
>>>>> strncpy(args.name, source, BTRFS_PATH_NAME_MAX);
>>>>> args.name[BTRFS_PATH_NAME_MAX] = ''\0'';
>>>>
>>>> That''s silly. Isn''t there a sane safe variant
of strcpy?
>>>
>>> There''s strlcpy, but it''s not in glibc because of
possible truncation
>>> errors!
>>
>> Then use a private wrapper.
>>
> 
> Here''s the new patch:
> 
> ----
> [PATCH] Add safe string manipulation functions
> 
> Deprecate direct use of strcpy(3)
> The following string manipulation function has been added:
> 
>    - string_copy() : wrapper of strcpy(3)
>    - string_ncopy(): wrapper of strncpy(3)
> 
> both function compose safe NULL terminated strings.
> ----
> 
> I check that the code most of the time raise an error if the path is too
> long, so the new wrappers should be ok...
> 
> best, 
> 
> Eduardo Silva
>  
[...]
+char *string_copy(char *dest, const char *src)
+{
+	if (!dest || !src) {
+		fprintf(stderr, "ERROR: invalid string_copy() parameters");
+		exit(EXIT_FAILURE);
+	}
+
+	memset(dest, ''\0'', sizeof(dest));

What is the purpose of the line above ?  sizeof(dest) is a const value
(typically 4 or 8) !

I agree with Olaf that string_copy() is usefulness.

I suggest you to improve the check of the string length before the copy
(not in the copy function), and raising an error when the length of the
string is too long.

Regards
G.Baroncelli
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On to, 2011-02-10 at 10:29 -0300, Eduardo Silva wrote:
> [PATCH] Add safe string manipulation functions
> 
> Deprecate direct use of strcpy(3)
> The following string manipulation function has been added:
> 
>    - string_copy() : wrapper of strcpy(3)
>    - string_ncopy(): wrapper of strncpy(3)
> 
> both function compose safe NULL terminated strings.
I''d like make some comments, which I hope will be acceptable.

Firstly, calling strcpy dangerous is, to me, rather overblown. It is
easy to make mistakes, but it is not at all dangerous the way, for
example, gets(3) is dangerous. strcpy can be used safely, gets cannot.
Also, if you consider strcpy to be dangerous, then strcat should be
dangerous too. However, given the risk of overwriting a buffer with
strcpy, I agree that it''s good to see if an alternative can be found.

Secondly, if you''re going to make wrappers or helper functions for
string handling in C, you need to decide several things right from the
start:

* do you do static or dynamic allocation?
* how do you handle errors?
* do you want a minimal wrapper or replacement, or a whole new library?

I am not familiar enough with the btrfs-progs code base to give any
strong recommendations, but off the top of my head I would suggest
these, for this patch:

* make use of fairly minimal wrappers/replacements (at least for now)
* handle errors by calling abort or exit
* don''t allocate data dynamically (or else it''s not a minimal
wrapper)

For error handling, there are two kinds of things that can happen:
normal run-time errors (malloc returns NULL, writing to a file fails,
etc), and programming errors (wrong parameters to functions). If we''re
doing a minimal wrapper without dynamic memory allocation, the only
thing string functions should need to worry about is programming errors.
For those, abort(3) is the appropriate way to terminate the program,
since it causes a core dump, which can be inspected with a debugger.
Since btrfs-progs are non-interactive command line tools, this should be
OK.

For checking function arguments, the assert macro is appropriate. It
calls abort if the test fails. I am not sure I would check for
parameters being non-NULL, though, since the kernel will trap such usage
and cause a segfault, which, again, can be analyzed with a debugger.

For things like string copying, another problem to consider is what to
do if the target array is not large enough? The two possibilities is to
silently truncate the output string, return an error code of some sort,
or to abort. The error code is a bit tedious, since it requires the
caller to check for it, and do something sensible if it''s not enough.
For btrfs-progs, I would suggest aborting.

Taking all of these together, my suggestion for a "safer strcpy" would
be along these lines (outline only, not tested code):

        void safer_strcpy(char *target, size_t tsize, const char
        *source)
        {
            size_t n;
        
            n = snprintf(target, tsize, "%s", source);
            assert(n < tsize);
        }
        
        void safer_strncpy(char *tgt, size_t tsize, const char *src, size_t n)
        {
            assert(n < tsize); /* There must be space for the
''\0''. */
            memset(tgt, ''\0'', tsize);
            strncpy(tgt, src, n);
        }

Note that for any reasonable error checking to be possible the safety
functions need to know the size of the target memory area. Otherwise no
sensible checks can be done -- you have to rely on the caller to check
that the target array is big enough, and then you''re nowhere better
than
with plain strcpy.

(Also note that I did not call the function string_copy, since global
names starting with str are reserved to the C implementation.)

Your function fills in the target array with zero bytes. Is that
necessary? If it is, then the memset call needs to be added to
safer_strcpy.

(I don''t find it useful to return the target array as the return value
of the function, so I didn''t do that.)

-- 
Blog/wiki/website hosting with ikiwiki (free for free software):
http://www.branchable.com/

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html