"Replace" function goes in overflow

Discussion in 'C Programming' started by pater, Dec 22, 2008.

  1. pater

    pater Guest

    I wrote this function by myself, but program crashes when this func
    gets called... why?

    ------

    void replace(char *data, char *what, char *with) {
    int i, ldata = strlen(data), lwhat = strlen(what), lwith = strlen
    (with);

    char *buffer = (char *) malloc(ldata);
    for(i=0;i<ldata-1;i++)
    if(strncmp(&data, what, strlen(what)) == 0) {
    if(lwhat<lwith) buffer = realloc(buffer, ldata+lwith-lwhat);
    memcpy(&buffer, with, lwith);
    i += lwith-1;
    } else memcpy(&buffer, &data, 1);

    memcpy(&buffer[ldata], 0, 1);
    data = buffer;
    }

    --------

    Some doubts then:
    1) malloc returns a pointer, no? so, the "buffer" var should contain
    the address of the heap address, shouldn't it?
    2) If so, memcpy wants a pointer as 1st and 2nd argument.. why should
    i pass &buffer instead of simply buffer?

    I have still some doubts about pointers... and reference/dereference
    operators..
    pater, Dec 22, 2008
    #1
    1. Advertising

  2. pater

    Richard Guest

    pater <> writes:

    > I wrote this function by myself, but program crashes when this func
    > gets called... why?
    >
    > ------
    >
    > void replace(char *data, char *what, char *with) {
    > int i, ldata = strlen(data), lwhat = strlen(what), lwith = strlen
    > (with);
    >
    > char *buffer = (char *) malloc(ldata);
    > for(i=0;i<ldata-1;i++)
    > if(strncmp(&data, what, strlen(what)) == 0) {
    > if(lwhat<lwith) buffer = realloc(buffer, ldata+lwith-lwhat);
    > memcpy(&buffer, with, lwith);
    > i += lwith-1;
    > } else memcpy(&buffer, &data, 1);
    >
    > memcpy(&buffer[ldata], 0, 1);
    > data = buffer;
    > }
    >
    > --------
    >
    > Some doubts then:
    > 1) malloc returns a pointer, no? so, the "buffer" var should contain
    > the address of the heap address, shouldn't it?
    > 2) If so, memcpy wants a pointer as 1st and 2nd argument.. why should
    > i pass &buffer instead of simply buffer?
    >
    > I have still some doubts about pointers... and reference/dereference
    > operators..


    Your post could well be a well constructed troll to get the usual regs
    tripping over themselves.

    Get rid of that horrible one liner where you compare
    lwhat & with and then realloc on a single line. Multiple statements on a
    single line are not friendly and are, invariably, harder to debug.

    Problems such as you have are as much about the way you think about
    programming as they are about the offending line.

    The first thing I note is that you realloc to make a bigger buffer but
    then dont reset ldata so for sure the memcpy at the end is wrong in some
    cases. The rest I would look at the locals in a debugger to discover
    .....

    Also: Look at your uses of strlen. Are you sure you want the
    strlen and not the buffer len? Its highly probable there are issues
    there. e.g strlen of "hello" is 5 but the buffer to hold it is 6
    long. That type of thing. The old n-1 issues. Also, reuse what you have
    : you have a lwhat but you dont use it in the strncmp line. Also, are
    you sure you want memcpy? Did you look at the manual for it? Did you
    possibly need memmove?

    Some sage advice:

    Show us that you have at least ATTEMPTED to debug it yourself using a
    debugger (ignore the people here who tell you debuggers are for losers -
    debuggers are there for a reason and can catch the crash and educate you
    quicker than you can post your question and then filter the replies).


    --
    I'm not a person who particularly had heros when growing up.
    - Dennis Ritchie when asked about the hero worship coming from c.l.c
    Richard, Dec 22, 2008
    #2
    1. Advertising

  3. pater

    Eric Sosman Guest

    pater wrote:
    > I wrote this function by myself, but program crashes when this func
    > gets called... why?
    >
    > ------
    >
    > void replace(char *data, char *what, char *with) {
    > int i, ldata = strlen(data), lwhat = strlen(what), lwith = strlen
    > (with);


    These would be better as size_t variables, but that's
    probably not a big deal unless the strings are rather long.

    > char *buffer = (char *) malloc(ldata);


    Three points: First, if you need to use a (char*) cast to
    silence a compiler complaint, it means you have forgotten to
    include <stdlib.h> and that silencing the complaint didn't fix
    the error. Second, malloc() will return NULL if it is unable
    to find enough memory for your request, and since you don't
    check for this possibility your program will misbehave if it
    ever happens. Third, I suspect you have forgotten to leave
    room for the '\0' that marks the end of a string.

    > for(i=0;i<ldata-1;i++)


    The "-1" looks bogus. (Thinks: Is it bogus? Yes, I'm
    quite sure it's bogus. Consider data="xy", what="y", and
    observe that the loop will never find the "y" in data.)

    > if(strncmp(&data, what, strlen(what)) == 0) {


    Since you've already computed lwhat, why not use it here?

    > if(lwhat<lwith) buffer = realloc(buffer, ldata+lwith-lwhat);


    Four points: First (and second), the length calculation (which
    still omits room for the trailing '\0') is correct only for the
    first replacement; after that, it fails to account for the size
    increase from replacing earlier what's by longer with's. Third,
    realloc() can fail just as malloc() can, and you should check for
    this. Fourth, if realloc() fails and you store NULL in buffer,
    you have just lost your only pointer to the still-allocated former
    memory area; this is called a "memory leak."

    > memcpy(&buffer, with, lwith);
    > i += lwith-1;


    This won't work unless lwhat==lwith. If they are unequal,
    then the number of characters deposited in buffer is not the
    same as the number of characters scanned in data; imagine
    data="xx", what="x", with="supercalifragilisticexpialidocious".
    You need two variables, one to keep track of the amount of
    progress in each of the arrays.

    > } else memcpy(&buffer, &data, 1);


    Why not a simple buffer=data? (But see above; it's
    probably i and j, not i and i.)

    > memcpy(&buffer[ldata], 0, 1);


    This is utterly wrong. It's trying to copy one character
    from the spot indicated by the second argument to the spot
    indicated by the first argument. What spot does the second
    argument point to? Nowhere!

    > data = buffer;


    This is legal, but ineffectual. The data variable is
    local to the function, and changing it does not affect the
    caller in any way. The caller's argument value initializes
    the data parameter, but thereafter they are unconnected.

    > }
    >
    > --------
    >
    > Some doubts then:
    > 1) malloc returns a pointer, no? so, the "buffer" var should contain
    > the address of the heap address, shouldn't it?
    > 2) If so, memcpy wants a pointer as 1st and 2nd argument.. why should
    > i pass &buffer instead of simply buffer?
    >
    > I have still some doubts about pointers... and reference/dereference
    > operators..


    The comp.lang.c Frequently Asked Questions (FAQ) site
    at <http://www.c-faq.com/> has good material on the topics
    you're having trouble with; read sections 6, 4, and 5 in
    conjunction with your textbook and things may become clearer.
    Also see sections 7 and 8 -- in fact, you should at least
    browse the entire FAQ, not to memorize what you find there
    but to be aware of the content so you'll remember to return
    to the FAQ next time you get into difficulty.

    --
    Eric Sosman
    lid
    Eric Sosman, Dec 22, 2008
    #3
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Brian Blais
    Replies:
    1
    Views:
    358
    Bruno Desthuilliers
    Jun 27, 2006
  2. Greg Ewing
    Replies:
    2
    Views:
    323
    Dieter Maurer
    Jun 29, 2006
  3. Alun
    Replies:
    3
    Views:
    4,462
    Masudur
    Feb 18, 2008
  4. bkaneweb

    Onchange function goes only once

    bkaneweb, Jun 14, 2007, in forum: Javascript
    Replies:
    1
    Views:
    60
    bkaneweb
    Jun 14, 2007
  5. V S Rawat
    Replies:
    5
    Views:
    268
    Richard Cornford
    Jul 3, 2007
Loading...

Share This Page