strange behaviour

[tfelb]

This function creates a puzzle for me which I don't understand.

Through the process of s2, the pointer increments beyond the limit of
the given pointer array in main.
I have another function where I used a pointer array as an argument
and a function parameter as a double pointer and this works!?

s2 must find the NULL after the second element of that pointer array
but it doesn't.

Sorry it's maybe a fool question.


Thanks for any help!

Tom F.



/*
* TEST Function
*/

void test(char *buffer, char *s1, char **s2)
{




while(*s2)
{

s2++; /* Steps beyond into garbage */
}




}

/* in main */

char ja[] = "This";
char jo[] = "That";
char je[] = "Nothing";


char *text[2];

text[0] = ja;
text[1] = jo;
text[2] = je;


join(erg,"<",text);

 

[Ben Bacarisse]

This function creates a puzzle for me which I don't understand.

void test(char *buffer, char *s1, char **s2)
{
while(*s2)
{
s2++; /* Steps beyond into garbage */
}
}

This loop (which I assume is sketch since it does no real work) must
be passed an array of char *s that, eventually, includes a null
pointer. Without one, the loop goes on eventually accessing pointers
that outside the array you pass. That is undefined behaviour.

/* in main */

char ja[] = "This";
char jo[] = "That";
char je[] = "Nothing";

char *text[2];

2 is not even big enough for the three pointers you assign to as the
elements of text! Change the 2 to 4 and add...

text[0] = ja;
text[1] = jo;
text[2] = je;

text[3] = NULL;

and you will be alright (at least as far as the example loop above is
concerned).

 

[Wolfgang Draxinger]

void test(char *buffer, char *s1, char **s2)
{
while(*s2)
{

s2++; /* Steps beyond into garbage */
}
}

Well, of course it does, if it get's not proper terminator. A
proper terminator was if *s2 evaluated to 0. So the terminating
element of the array you give as s2 should be a null pointer.

/* in main */

char ja[] = "This";
char jo[] = "That";
char je[] = "Nothing";


char *text[2];

When defining an array the value between the brackets is the
_count_ of elements, the array holds. So to hold 3 values you
must write:

char *text[3];

text[0] = ja;
text[1] = jo;
text[2] = je;

Here you're indexing. Indexing can be understood as

first element + index

So to get the first element you use index 0, for the second
element index 1...

join(erg,"<",text);

Well, you didn't tell us, how "join" is defines, but the way you
call it and the way it crashes allows for some /assumptions/
(not statements, let's make this straight):

join looks like some function, that (I presume it from the name)
concatenates a array of strings into a single string, putting
the string defined in the second argument between the parts.

To indicate, that the last element has been reached, a NULL
pointer shall be provided as the terminating (in most cases this
is the last) array element.

The function "test" supports this assumptions, as it's while loop
exhibits the very same termination logic. So to be correct, your
main-code should be:

char ja[] = "This";
char jo[] = "That";
char je[] = "Nothing";

char *text[4];

text[0] = ja;
text[1] = jo;
text[2] = je;
text[3] = NULL; /* NULL == 0 BTW */

join(erg, "<", text)


NOTE: The interface of "join" and "test" are both inerhently
unsafe: They both assume, that the 'buffer' argument points to a
memory location with enough space allocated to hold the result.

Even if the functions have a hard coded limit, so that you can't
overrun the buffer with 'overlong' strings in the 's2' argument,
it's still possible to give a too short buffer, thus causing a
overrun. Buffer overruns have been the most common cause for
security holes, as they allow to inject code into a program
(google for "buffer overrun exploit techniques").

Wolfgang Draxinger

 

[tfelb]

void test(char *buffer, char *s1, char **s2)
{
     while(*s2)
{

s2++; /* Steps beyond into garbage */
}
}

Well, of course it does, if it get's not proper terminator. A
proper terminator was if *s2 evaluated to 0. So the terminating
element of the array you give as s2 should be a null pointer.

/* in main */

char ja[] = "This";
char jo[] = "That";
                char je[] = "Nothing";

                char *text[2];

When defining an array the value between the brackets is the
_count_ of elements, the array holds. So to hold 3 values you
must write:

char *text[3];

text[0] = ja;
text[1] = jo;
text[2] = je;

Here you're indexing. Indexing can be understood as

first element + index

So to get the first element you use index 0, for the second
element index 1...

join(erg,"<",text);

Well, you didn't tell us, how "join" is defines, but the way you
call it and the way it crashes allows for some /assumptions/
(not statements, let's make this straight):

join looks like some function, that (I presume it from the name)
concatenates a array of strings into a single string, putting
the string defined in the second argument between the parts.

To indicate, that the last element has been reached, a NULL
pointer shall be provided as the terminating (in most cases this
is the last) array element.

The function "test" supports this assumptions, as it's while loop
exhibits the very same termination logic. So to be correct, your
main-code should be:

char ja[] = "This";
char jo[] = "That";
char je[] = "Nothing";

char *text[4];

text[0] = ja;
text[1] = jo;
text[2] = je;
text[3] = NULL; /* NULL == 0 BTW */

join(erg, "<", text)

NOTE: The interface of "join" and "test" are both inerhently
unsafe: They both assume, that the 'buffer' argument points to a
memory location with enough space allocated to hold the result.

Even if the functions have a hard coded limit, so that you can't
overrun the buffer with 'overlong' strings in the 's2' argument,
it's still possible to give a too short buffer, thus causing a
overrun. Buffer overruns have been the most common cause for
security holes, as they allow to inject code into a program
(google for "buffer overrun exploit techniques").

Wolfgang Draxinger

Thanks! This code is in /pre/ stadium so i know it doesn't have any
security checks but i think its better to use a pointer array as an
argument instead of a double pointer.

Now it works.

 

[Wolfgang Draxinger]

Thanks! This code is in /pre/ stadium so i know it doesn't have
any security checks

Epic failure!

Security is difficult to add later in existing systems. Always, I
repeat, always design interfaces in a

but i think its better to use a pointer
array as an argument instead of a double pointer.

You mean something like

char *s2[]

? Technically it behaves exactly like char **s2, though at
compilation stage the types subtely differ. However this is
something of interest for compiler writers, the average coder
should know, that the "pointer to pointer" notation is almost
the same as the "array of pointers" notation and is identical,
when it comes to machine language.

Now it works.

It works in if well formulated data is input. A much safer
interface would be:

void join(
unsigned int const dst_buffer_length,
char * const dst_buffer,
char const * const delimiter_string,
unsigned int const src_string_count
char const * const * const src_strings,
);


A even more safer style would completely abandon the use
of "naked" arrays and instead encapsulate every array operation
within bounds checking helper functions. Have a look at Felix
von Leitner's libowfat library: It provides such an array
abstraction - together with things like overflow safe integer
multiplication and other neat stuff.
<http://www.fefe.de/libowfat/>

Just to give you an idea how it works with arrays, this is an
excerpt of "array.h" of libowfat:

#ifndef ARRAY_H
#define ARRAY_H

#include "uint64.h"

typedef struct {
char* p;
int64 allocated; /* in bytes */
uint64 initialized; /* in bytes */

/* p and allocated nonzero: array is allocated */
/* p and allocated zero: array is unallocated */
/* p zero and allocated < 0: array is failed */
} array;

void* array_allocate(array* x,uint64 membersize,int64 pos);
void* array_get(array* x,uint64 membersize,int64 pos);
void* array_start(const array* const x);
int64 array_length(const array* const x,uint64 membersize);
int64 array_bytes(const array* const x);
void array_truncate(array* x,uint64 membersize,int64 len);
void array_trunc(array* x);
void array_reset(array* x);
void array_fail(array* x);
/* ... */

uint64.h is the header to overflow safe multiplication code and
associated types (it wont resort to bignums, if a multiplication
overflows, but such is detected and triggers error handling).
Thow only "drawback" of libowfat is, that it's GPL licenced, so
you can't use it in a closed source project. But you're still
free to have a look at it's code and get inspiration from it.

Wolfgang Draxinger

 

[Keith Thompson]

tfelb wrote: [...]

but i think its better to use a pointer
array as an argument instead of a double pointer.

(Note: "double pointer" here means pointer to pointer, not type
double*.)

You mean something like

char *s2[]

? Technically it behaves exactly like char **s2, though at
compilation stage the types subtely differ. However this is
something of interest for compiler writers, the average coder
should know, that the "pointer to pointer" notation is almost
the same as the "array of pointers" notation and is identical,
when it comes to machine language.

[...]

It's actually simpler than that. The parameter declaration

char *s2[];

is identical to

char **s2;

It's not "almost the same"; it is the same, not just in machine
language but in C. Obviously the compiler has to handle the
translation of the former to the latter.

(Other than in a parameter declaration, of course, pointers and arrays
are very different, though some features of the language seemingly
conspire to hide that fact. Section 6 of the comp.lang.c FAQ explains
this well.)