Randomly crashing program. Bad pointer to realloc?

[tjay]

Hi all,

I wrote a function (dir_iterate) to iterate over the contents of a
directory. The problem is, sometimes it works, and sometimes it
doesn't... When it doesn't work, I'll get this error message:

*** glibc detected *** realloc(): invalid next size: 0x0806b038 ***

Running the program with MALLOC_CHECK_="1" gives me the following
output (Lines beginning with "Error: " are ok):

MALLOC_CHECK_="1" PIPER_PLUGIN_DIR=".:testdir" ./piper

malloc: using debugging hooks
Error: Failed to load plugin: ./. - Filename invalid or does not end in
".ppg".
Error: Failed to load plugin: ./.. - Filename invalid or does not end
in ".ppg".
....
Error: Failed to load plugin: testdir/.keep - Filename invalid or does
not end in ".ppg".
Error: Failed to load plugin: testdir/libXCBrecord.so.0 - Filename
invalid or does not end in ".ppg".
Error: Failed to load plugin: - Filename invalid or does not end in
".ppg".
*** glibc detected *** realloc(): invalid pointer: 0x0806b040 ***
Error: Failed to load plugin: - Filename invalid or does not end in
".ppg".
Error: Failed to load plugin: - Filename invalid or does not end in
".ppg".
*** glibc detected *** realloc(): invalid pointer: 0x0806b040 ***
*** glibc detected *** malloc(): memory corruption (fast): 0x0806b060
***
Segmentation fault

First the filename fails to print, then I get a warning from glibc, and
then it segfaults.

I'm on Linux 2.6, with GCC 3.4.4.
Here's my program (formatted to fit into 80 columns):
(You have to define PIPER_PLUGIN_DIR befor running it. The program will
try to dlopen all the files in the directory.)

/*===================================================================
License */
/*
Piper
Copyright (C) 2006 Woon Tien Jing

This file is part of Piper.

Piper is free software; you can redistribute it and/or modify
it under the terms of the GNU General Public License as published by
the Free Software Foundation; either version 2 of the License, or
(at your option) any later version.

Piper is distributed in the hope that it will be useful,
but WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
GNU General Public License for more details.

You should have received a copy of the GNU General Public License
along with Piper; if not, write to the Free Software
Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
USA
*/

/*=====================================================================
Notes */
/*
Written in Standard C 89.
*/
/*
BUGS:
- Plugin loading/directory iterating fails randomly. Error message:
*** glibc detected *** realloc(): invalid next size: 0x0806b038 ***
Do a 'MALLOC_CHECK_="1"' before running Piper to see what's
happening.
*/

/*==================================================================
Includes */

/* C Standard Library */
#include <stdlib.h>
#include <stdio.h>
#include <stdarg.h>
#include <string.h>

/* POSIX */
#include <dirent.h>
#include <dlfcn.h>

#include "piper.h"

/*========================================================== Type
definitions */

typedef struct {
/* Essential */
char *id;
int ver;
/* Internal */
void *lib;
/* Documentation */
char *name;
char *path;
} plugin_t;

/*========================================================== Global
variables */

static plugin_t *plugins;
static int num_of_loaded_plugins;

/*============================================================ Misc
functions */

static void _printerror (char *prefix, char *msg, va_list args) {
fprintf(stderr, prefix);
vfprintf(stderr, msg, args);
fprintf(stderr, "\n");
}

static void fatalerror (char *msg, ...) {
va_list args;

va_start(args, msg);
_printerror("Fatal error: ", msg, args);
va_end(args);
exit(EXIT_FAILURE);
}

static void error (char *msg, ...) {
va_list args;

va_start(args, msg);
_printerror("Error: ", msg, args);
va_end(args);
}

static int dir_iterate (char *dir_path,
void (*fn) (char *path, void *userdata),
void *userdata) {
/* Returns 0 on success;
1 on failure to open directory;
2 on out-of-memory; */

DIR *dir;
struct dirent *current;
int dir_path_len;
char *tmp;

if(!(dir=opendir(dir_path))) {
error("(dir_iterate) Failed to open directory %s.", dir_path);
return 1;
}

dir_path_len = strlen(dir_path);
if(dir_path[dir_path_len-1] != '/') { /* If doesn't end in directory
separator */
tmp = malloc(dir_path_len + 2); /* Alloc space for a separator */
dir_path_len++;
}
else {
tmp = malloc(dir_path_len + 1);
}
strcpy(tmp, dir_path);
tmp[dir_path_len-1] = '/'; /* Make sure path ends in a directory
separator */
while(current=readdir(dir)) {
int newsize = strlen(current->d_name) + dir_path_len + 1;
if(!realloc(tmp, newsize)) {
error("(dir_iterate) Ran out of memory while iterating over %s.",
dir_path);
return 2;
}
strcpy(&tmp[dir_path_len], current->d_name);
fn(tmp, userdata);
}
free(tmp);

closedir(dir);
return 0;
}

/*============================================================ Plugin
loading */

static void _plugin_loadfail (char *path, char *err) {
error("Failed to load plugin: %s - %s",
path, err);
}

static void _loadplugin (char *path, void *countp) {
/* Only loads plugin if path ends in ".ppg" */
plugin_t p;
void *lib;
char *name, *id;
int *reqpiperver;

if(strlen(path) <= 4 || strcmp(&path[strlen(path)-4], ".ppg")) {
_plugin_loadfail(path, "Filename does not end in \".ppg\".");
return;
}

if(!(lib = dlopen(path, RTLD_NOW | RTLD_LOCAL))) {
_plugin_loadfail(path, dlerror());
return;
}

if(!(reqpiperver = dlsym(lib, "REQ_PIPER_VER"))) {
_plugin_loadfail(path, "REQ_PIPER_VER not found.");
return;
}
if(*reqpiperver > PIPER_VERSION ||
*reqpiperver/100000 != PIPER_VERSION/100000 /* wrong major version
*/) {
_plugin_loadfail(path, "Piper version does not match plugin.");
return;
}

if(!(name = dlsym(lib, "NAME"))) {
_plugin_loadfail(path, "NAME not found");
return;
}
p.name = malloc(strlen(name)+1);
strcpy(p.name, name);

if(!(id = dlsym(lib, "UUID"))) {
_plugin_loadfail(path, "UUID not found");
return;
}
if(!(p.id = malloc(strlen(id)+1))) {
_plugin_loadfail(path, "Ran out of memory.");
return;
}
strcpy(p.id, id);

if(!(reqpiperver = dlsym(lib, "VERSION"))) { /* Re-use reqpiperver...
*/
_plugin_loadfail(path, "VERSION not found.");
return;
}
p.ver = *reqpiperver;

if(!(p.path = malloc(strlen(path) + 1))) {
_plugin_loadfail(path, "Ran out of memory.");
return;
}
strcpy(p.path, path);

p.lib = lib;

if(!(plugins = realloc(plugins, (*(int*)countp+1)*sizeof(plugin_t))))
{
_plugin_loadfail(path, "Ran out of memory.");
return;
}

/* Success */
memcpy(&plugins[*(int*)countp], &p, sizeof(plugin_t));
++*(int*)countp;
}

static void _collectplugins (void) {
/* Loads all plugins in PIPER_PLUGIN_DIR into the global 'plugins'.
*/
char *plugin_dir, *tmp;
int count = 0;

if(!(plugin_dir = getenv("PIPER_PLUGIN_DIR"))) {
fatalerror("PIPER_PLUGIN_DIR is not defined");
}

if(!(tmp = malloc(strlen(plugin_dir) + 1))) {
fatalerror("(loadplugins) Ran out of memory");
}
strcpy(tmp, plugin_dir);

plugin_dir = strtok(tmp, ":");
do {
dir_iterate(plugin_dir, _loadplugin, &count);
} while(plugin_dir = strtok(0, ":"));

num_of_loaded_plugins = count;
}

static void _initplugins (void) {
plugin_t *loaded, *p;

loaded = plugins;
plugins = 0;

for(p=loaded; p < &loaded[num_of_loaded_plugins]; p++) {

}
}

static void loadplugins (void) {
_collectplugins();
_initplugins();
}

/*======================================================================
Main */

int main (int argc, char **argv) {
loadplugins();

return 0;
}

 

[David Resnick]

Hi all,

I wrote a function (dir_iterate) to iterate over the contents of a
directory. The problem is, sometimes it works, and sometimes it
doesn't... When it doesn't work, I'll get this error message:
....

if(!realloc(tmp, newsize)) {

error("(dir_iterate) Ran out of memory while iterating over %s.",
dir_path);
return 2;
}
strcpy(&tmp[dir_path_len], current->d_name);

If the realloc DOES work and returns a new pointer value, you've
dropped that new pointer it returns onto the floor. And the original
pointer held in tmp is no longer valid. You want to do something like

void *tmp2 = realloc(tmp,newsize);

if (tmp2 == NULL) {
/* handle error */
}
else {
tmp = tmp2;
}

You need do that, because if you do

tmp = realloc(tmp,newsize); /* BAD */

and the realloc fails, the original tmp will be leaked...

-David

 

[Ancient_Hacker]

Hi all,

I wrote a function (dir_iterate) to iterate over the contents of a
directory. The problem is, sometimes it works, and sometimes it
doesn't...

It might help to use the safer string functions, strncpy, et. al.

It's really easy to be off by one byte, or fall thru the wrong path and
clobber unallocated memory.

 

[Keith Thompson]

It might help to use the safer string functions, strncpy, et. al.

It's really easy to be off by one byte, or fall thru the wrong path and
clobber unallocated memory.

strncpy() is rarely what you want. If the target is too small, the
result is not terminated with a '\0' character (and is therefore not a
string). If the target is too big, it's padded with multiple '\0'
characters, which is rarely necessary.

A better approach is to first check whether the target buffer is big
enough, and then use strcpy() or even memcpy() (and don't make
mistakes).

strncat() behaves more sensibly, as do most of the functions with 'n'
added to their names; strncpy() is an anomaly.

 

[tjay]

If the realloc DOES work and returns a new pointer value, you've
dropped that new pointer it returns onto the floor. And the original
pointer held in tmp is no longer valid. You want to do something like

void *tmp2 = realloc(tmp,newsize);

if (tmp2 == NULL) {
/* handle error */
}
else {
tmp = tmp2;
}

You need do that, because if you do

tmp = realloc(tmp,newsize); /* BAD */

and the realloc fails, the original tmp will be leaked...

-David

Oh my, so that's how realloc works! I thought it would modify tmp for
me!
Thanks a lot! You're a lifesaver

Ancient_Hacker:

It might help to use the safer string functions, strncpy, et. al.
...

I'm already checking the required size of the buffer and allocating
enough memory for it. I don't understand how this can be a problem

The only way this can fail (as far as I can see) is if strlen returns
jibberish. And that means either the string is longer than an int can
hold, or the string is not null-terminated, which shouldn't be possible
for a filename, correct?

Keith Thompson:

A better approach is to first check whether the target buffer is big
enough, and then use strcpy() or even memcpy() (and don't make
mistakes).

That's what I did in my program. Do you see any mistakes, other than
the silly issue with realloc?

Thanks!

tj

 

[Keith Thompson]

Oh my, so that's how realloc works! I thought it would modify tmp for
me!

If you look at the way realloc() is declared and called, you'll see
that it *can't* modify tmp. When you call realloc:

realloc(tmp, newsize);

you pass it the *value* of tmp. realloc() doesn't have the address of
tmp, so there's no way it can modify it. (If realloc() were a macro
rather than a function, it could do so, but it isn't -- or rather, it
can be implemented as a macro, but it has to behave like a function.)

[...]

Keith Thompson:

That's what I did in my program. Do you see any mistakes, other than
the silly issue with realloc?

Sorry, I didn't look very closely at the program; I just noticed
something in one of the responses.