Hello All,

I don't know if I have the right group here, so if I have the wrong
group please point me in the right direction, if its not too much trouble.

I've started learning C because as a C# programmer by day-job, I felt I
wasn't getting enough exposure to the underlying hardware in the machine.

I think C would help me understand programming at a deeper level, so I
decided to write my small web-site entirely in C using Apache and CGI.

I understand that some people might consider that lunacy - and they'd be
right - but there's no point in doing basic tutorials when you can get
yourself in to something a little meatier!

So I've picked Sqllite3 as my back-end database and the code snippet I'm
about to submit is for the RSS feed.

There are a lot of things that probably aren't too smart in this code.
For a start, there's a lot of return values I don't check and there are
some "crazy" decisions in there. One being that I've built a basic
string replace function. A lot what's in there is probably junk. I'm
hoping you can tell me why.

I'm having a problem that when I try to free a malloc called from within
another function, it causes a segfault. I've heard that Valgrind is the
tool to use to debug these sorts of things. When I run it I get 15 or so
errors and I'm pretty sure these errors are responsible for the
segfault. But I have no idea how to fix them, even though I roughly know
what they mean.

If you could be so kind, could somebody help me try and understand
what's going on?

Source-Code:

#include <stdio.h>
#include <stdlib.h>
#include <time.h>
#include <sqlite3.h>
#include <string.h>

char* replace(register char* source_str,char search_char,char* replace_str)
{
unsigned int nstr_allocated=0;

if(!source_str || !search_char || !replace_str)
{
printf("Not enough arguments\n");
return NULL;
}

int source_length = strlen(source_str);

nstr_allocated = sizeof(char) * (source_length+1);

register char *nstr;
nstr = (char*) malloc( sizeof(char) * (source_length+1));

register const char* oldStrCursor = source_str;
register char* newStrCursor = nstr;

int counter = 0;

while(counter < source_length)
{
if (search_char  == *oldStrCursor)
{
nstr_allocated += strlen(replace_str) + 1;
nstr = realloc(nstr, nstr_allocated);

char* replaceCursor = replace_str;
int i = 0;

for (i=0; i < strlen(replace_str); i++)
{
*newStrCursor = *replaceCursor;
newStrCursor++;
replaceCursor++;
}
}
else
{
*newStrCursor = *oldStrCursor;
newStrCursor++;
}

oldStrCursor++;
counter++;
}

*newStrCursor = '\0';
return nstr;
}

void DrawDescription(sqlite3_stmt* statement)
{
printf("\t\t\t<description>\n");


char* charPointer = (char*) sqlite3_column_text(statement, 1);


char invCharacters[4] = { '&', '<', '>', '\"' };

char replacements[][8] = {"&", "&lt;", "&gt;", """ };

int counter = 0;

for (counter; counter < 4; counter++ )
{
char* lastReference = charPointer;

charPointer = (char*) replace(charPointer, invCharacters[counter],
(char*) replacements[counter]);

free(lastReference); // Remove this line and the program always works.

}

printf("%s\n", charPointer);

free(charPointer);

printf("\t\t\t</description>\n");

}

int DrawTitle(sqlite3_stmt* statement)
{
char* dateString = (char*) sqlite3_column_text(statement, 2);

struct tm tim;

if (strptime(dateString, "%Y-%m-%d %H:%M:%S", &tim)==0)
{
printf("<p><b>Error:</b> Couldn't parse date-time column.</p>");
return 1;
}

char blogDateString[60];

strftime( (char*) &blogDateString, 60, "%A, %B %d, %Y", &tim);

printf("\t\t\t<title>%s</title>\n", blogDateString);

return 0;

}

int DrawPublishDate(sqlite3_stmt* statement)
{
char* dateString = (char*) sqlite3_column_text(statement, 2);

struct tm tim;

if (strptime(dateString, "%Y-%m-%d %H:%M:%S", &tim)==0)
{
printf("<p><b>Error:</b> Couldn't parse date-time column.</p>");
return 1;
}

char blogDateString[60];

strftime( (char*) &blogDateString, 60, "%a, %d %b %Y %H:%M:%S", &tim);

printf("\t\t\t<pubDate>%s GMT</pubDate>\n", blogDateString);

return 0;

}

void WriteItems()
{

sqlite3* dbStructure;

int didOpen = sqlite3_open("/home/ckwop/Website.db", &dbStructure);

sqlite3_stmt* statement = NULL;

int errorCode =	sqlite3_prepare(dbStructure, "select BlogId,
BlogContents, CreatedDate from Blog where Display = 1 order by BlogId
desc limit 10;", -1, &statement, 0);

if ( errorCode != SQLITE_OK)
{
printf("<p><b>Error: </b>%s</p>", sqlite3_errmsg(dbStructure));
}

while (errorCode == SQLITE_OK && sqlite3_step(statement) == SQLITE_ROW)
{
int blogId = sqlite3_column_int(statement, 0);

printf("\t\t<item>\n");

DrawTitle(statement);

DrawDescription(statement);


printf("\t\t\t<link>http://www.ckwop.me.uk/default.asp?Page=%d</link>\n",
blogId);

if (DrawPublishDate(statement) == 1)
{
goto cleanup;
}

printf("\t\t\t<guid
isPermaLink=\"true\">http://www.ckwop.me.uk/default.asp?Page=%d</guid>\n",
blogId);
printf("\t\t</item>\n");
}


cleanup:
sqlite3_close(dbStructure);
sqlite3_free(statement);
sqlite3_free(dbStructure);

}


int main(void)
{
printf("Content-Type: text/xml;\n\n");
printf("<?xml version=\"1.0\" ?>\n");
printf("<rss version=\"2.0\">\n");
printf("\t<channel>\n");
printf("\t\t<title>Ckwop's Journal</title>\n");
printf("\t\t<link>http://www.ckwop.me.uk/</link>\n");
printf("\t\t<description>The life of Simon.. A journal on Life,
Programing and Maths</description>\n");
printf("\t\t<copyright>Copyright 2003-2007 Simon Johnson</copyright>\n");
printf("\t\t<language>en-gb</language>\n");
printf("\t\t<generator>C code by Simon</generator>\n");
printf("\t\t<managingEditor>me@ckwop.me.uk (Simon
Johnson)</managingEditor>\n");
printf("\t\t<ttl>60</ttl>\n");
printf("\t\t<lastBuildDate>Fri, 20 Apr 2007 20:14:29 GMT
</lastBuildDate>\n");

WriteItems();

printf("\t</channel>\n");
printf("</rss>");
}

Valgrind output:

==10841== 16 errors in context 3 of 5:
==10841== Invalid write of size 1
==10841==    at 0x8048967: replace (rss.c:56)
==10841==    by 0x8048A45: DrawDescription (rss.c:77)
==10841==    by 0x8048C9E: WriteItems (rss.c:159)
==10841==    by 0x8048DCF: main (rss.c:197)
==10841==  Address 0x4201D8B is 43 bytes inside a block of size 11,254
free'd
==10841==    at 0x402171B: realloc (vg_replace_malloc.c:306)
==10841==    by 0x80488EA: replace (rss.c:34)
==10841==    by 0x8048A45: DrawDescription (rss.c:77)
==10841==    by 0x8048C9E: WriteItems (rss.c:159)
==10841==    by 0x8048DCF: main (rss.c:197)
==10841==
==10841== 1413 errors in context 4 of 5:
==10841== Invalid write of size 1
==10841==    at 0x804890D: replace (rss.c:41)
==10841==    by 0x8048A45: DrawDescription (rss.c:77)
==10841==    by 0x8048C9E: WriteItems (rss.c:159)
==10841==    by 0x8048DCF: main (rss.c:197)
==10841==  Address 0x41FF432 is 754 bytes inside a block of size 11,248
free'd
==10841==    at 0x402171B: realloc (vg_replace_malloc.c:306)
==10841==    by 0x80488EA: replace (rss.c:34)
==10841==    by 0x8048A45: DrawDescription (rss.c:77)
==10841==    by 0x8048C9E: WriteItems (rss.c:159)
==10841==    by 0x8048DCF: main (rss.c:197)
==10841==
==10841== 55444 errors in context 5 of 5:
==10841== Invalid write of size 1
==10841==    at 0x804894A: replace (rss.c:48)
==10841==    by 0x8048A45: DrawDescription (rss.c:77)
==10841==    by 0x8048C9E: WriteItems (rss.c:159)
==10841==    by 0x8048DCF: main (rss.c:197)
==10841==  Address 0x41FF437 is 759 bytes inside a block of size 11,248
free'd
==10841==    at 0x402171B: realloc (vg_replace_malloc.c:306)
==10841==    by 0x80488EA: replace (rss.c:34)
==10841==    by 0x8048A45: DrawDescription (rss.c:77)
==10841==    by 0x8048C9E: WriteItems (rss.c:159)
==10841==    by 0x8048DCF: main (rss.c:197)

Thanks,

Simon Johnson

[ Post a follow-up to this message ]

See comments embedded into your source...

> Source-Code:
>
> #include <stdio.h>
> #include <stdlib.h>
> #include <time.h>
> #include <sqlite3.h>
> #include <string.h>
>
> char* replace(register char* source_str,char search_char,char* replace_str
)
> {
>     unsigned int nstr_allocated=0;
>
>     if(!source_str || !search_char || !replace_str)
>     {
>         printf("Not enough arguments\n");
>         return NULL;
>     }
>
>     int source_length = strlen(source_str);
>
>     nstr_allocated = sizeof(char) * (source_length+1);
>
>     register char *nstr;
>     nstr = (char*) malloc( sizeof(char) * (source_length+1));
>
>     register const char* oldStrCursor = source_str;
>     register char* newStrCursor = nstr;

Ok, so now newStrCursor points to nstr.

>
>     int counter = 0;
>
>     while(counter < source_length)
>     {
>         if (search_char  == *oldStrCursor)
>         {
>             nstr_allocated += strlen(replace_str) + 1;
>             nstr = realloc(nstr, nstr_allocated);

Two potential problems ...
1) if realloc fails, it returns NULL ... by using nstr twice here,
you would lose the original content ... memory hole
2) nstr will (not too unlikely change) ... now nstr is new, but
newStrCursor still points to the old value ... this is asking for trouble.

>
>             char* replaceCursor = replace_str;
>             int i = 0;
>
>             for (i=0; i < strlen(replace_str); i++)
>             {
>                 *newStrCursor = *replaceCursor;
>                 newStrCursor++;
>                 replaceCursor++;
>             }
>         }
>         else
>         {
>             *newStrCursor = *oldStrCursor;
>             newStrCursor++;
>         }
>
>         oldStrCursor++;
>         counter++;
>     }
>
>     *newStrCursor = '\0';
>     return nstr;
> }
>
> void DrawDescription(sqlite3_stmt* statement)
> {
>     printf("\t\t\t<description>\n");
>
>
>     char* charPointer = (char*) sqlite3_column_text(statement, 1);

Since you free this charPointer (via the copy in lastReference
in the loop), I take it that sqlite3_column_text returns something
allocated, and wants you to free that?

>
>
>     char invCharacters[4] = { '&', '<', '>', '\"' };
>
>     char replacements[][8] = {"&", "&lt;", "&gt;", ""
;" };
>
>     int counter = 0;
>
>     for (counter; counter < 4; counter++ )
>     {
>         char* lastReference = charPointer;
>
>         charPointer = (char*) replace(charPointer,
> invCharacters[counter], (char*) replacements[counter]);
>
>         free(lastReference); // Remove this line and the program always
> works.
>
>     }
>
>     printf("%s\n", charPointer);
>
>     free(charPointer);
>
>     printf("\t\t\t</description>\n");
>
> }
>
> int DrawTitle(sqlite3_stmt* statement)
> {
>     char* dateString = (char*) sqlite3_column_text(statement, 2);
>
>     struct tm tim;
>
>     if (strptime(dateString, "%Y-%m-%d %H:%M:%S", &tim)==0)
>     {
>         printf("<p><b>Error:</b> Couldn't parse date-time column.</p>");
>         return 1;
>     }
>
>     char blogDateString[60];
>
>     strftime( (char*) &blogDateString, 60, "%A, %B %d, %Y", &tim);
>
>     printf("\t\t\t<title>%s</title>\n", blogDateString);
>

If sqlite3_column_text wants you to free the result ... you now
have a memory hole, or an error in the previous routine.

>     return 0;
>
> }
>
> int DrawPublishDate(sqlite3_stmt* statement)
> {
>     char* dateString = (char*) sqlite3_column_text(statement, 2);
>
>     struct tm tim;
>
>     if (strptime(dateString, "%Y-%m-%d %H:%M:%S", &tim)==0)
>     {
>         printf("<p><b>Error:</b> Couldn't parse date-time column.</p>");
>         return 1;
>     }
>
>     char blogDateString[60];
>
>     strftime( (char*) &blogDateString, 60, "%a, %d %b %Y %H:%M:%S", &tim);
>
>     printf("\t\t\t<pubDate>%s GMT</pubDate>\n", blogDateString);
>

Same here

>     return 0;
>
> }
>
> void WriteItems()
> {
>
>     sqlite3* dbStructure;
>
>     int didOpen = sqlite3_open("/home/ckwop/Website.db", &dbStructure);
>
>     sqlite3_stmt* statement = NULL;
>
>     int errorCode =    sqlite3_prepare(dbStructure, "select BlogId,
> BlogContents, CreatedDate from Blog where Display = 1 order by BlogId
> desc limit 10;", -1, &statement, 0);
>
>     if ( errorCode != SQLITE_OK)
>     {
>         printf("<p><b>Error: </b>%s</p>", sqlite3_errmsg(dbStructure));
>     }
>
>     while (errorCode == SQLITE_OK && sqlite3_step(statement) == SQLITE_ROW
)
>     {
>         int blogId = sqlite3_column_int(statement, 0);
>
>         printf("\t\t<item>\n");
>
>         DrawTitle(statement);
>
>         DrawDescription(statement);
>
>
> printf("\t\t\t<link>http://www.ckwop.me.uk/default.asp?Page=%d</link>\n",
> blogId);
>
>         if (DrawPublishDate(statement) == 1)
>         {
>             goto cleanup;
>         }
>
>         printf("\t\t\t<guid
> isPermaLink=\"true\">http://www.ckwop.me.uk/default.asp?Page=%d</guid>\n",
> blogId);
>         printf("\t\t</item>\n");
>     }
>
>
>     cleanup:
>     sqlite3_close(dbStructure);
>     sqlite3_free(statement);
>     sqlite3_free(dbStructure);
>
> }
>
>
> int main(void)
> {
>     printf("Content-Type: text/xml;\n\n");
>     printf("<?xml version=\"1.0\" ?>\n");
>     printf("<rss version=\"2.0\">\n");
>     printf("\t<channel>\n");
>     printf("\t\t<title>Ckwop's Journal</title>\n");
>     printf("\t\t<link>http://www.ckwop.me.uk/</link>\n");
>     printf("\t\t<description>The life of Simon.. A journal on Life,
> Programing and Maths</description>\n");
>     printf("\t\t<copyright>Copyright 2003-2007 Simon
> Johnson</copyright>\n");
>     printf("\t\t<language>en-gb</language>\n");
>     printf("\t\t<generator>C code by Simon</generator>\n");
>     printf("\t\t<managingEditor>me@ckwop.me.uk (Simon
> Johnson)</managingEditor>\n");
>     printf("\t\t<ttl>60</ttl>\n");
>     printf("\t\t<lastBuildDate>Fri, 20 Apr 2007 20:14:29 GMT
> </lastBuildDate>\n");
>
>     WriteItems();
>
>     printf("\t</channel>\n");
>     printf("</rss>");
> }

As you see, you pretty much messed arround in heap memory just with a
few statements. For shortliving programs, it might eventually be better
not to free up memory at all. (Its freed up with program termination).

If you don't want to care about allocated memory, a language like java
with an intrinsic garbage collector is easier to handle. In C you have
to be careful with self-allocated memory.

[ Post a follow-up to this message ]

Simon Johnson wrote:
> I don't know if I have the right group here, so if I have the wrong
> group please point me in the right direction, if its not too much trouble.
>
> I've started learning C because as a C# programmer by day-job, I felt I
> wasn't getting enough exposure to the underlying hardware in the machine.

I'd suggest alt.comp.lang.learn.c-c++ and the FAQ from comp.lang.c.

> There are a lot of things that probably aren't too smart in this code.
> For a start, there's a lot of return values I don't check

If the only sensible way to handle an error is to terminate a program, do
so. For that, you write a simple wrapper function:

FILE* xfopen( char const* filename) {
FILE* f = fopen(filename);
if(!f)
error("failed to open file");
return f;
}

The used error() function of course never returns but calls
exit(EXIT_FAILURE). Note that using C++ or Java (I'm wondering about C#..)
you would actually throw an exception instead and have the same result,
application logic isn't cluttered by error-handling code.

Uli

[ Post a follow-up to this message ]

On 26 Jun, 05:29, Ulrich Eckhardt <dooms...@knuut.de> wrote:

[]

> If the only sensible way to handle an error is to terminate a program, do
> so. For that, you write a simple wrapper function:
>
> FILE* xfopen( char const* filename) {
>   FILE* f = fopen(filename);
>   if(!f)
>     error("failed to open file");
>   return f;
>
> }
>
> The used error() function of course never returns but calls
> exit(EXIT_FAILURE). Note that using C++ or Java (I'm wondering about C#..)
> you would actually throw an exception instead and have the same result,
> application logic isn't cluttered by error-handling code.

Unhandled C++ exception causes a call to std::terminate(), which
invokes abort() by default. abort() dumps core preserving the state of
the application for investigation, on the other hand,
exit(EXIT_FAILURE) does not dump core and it does call atexit()
handlers (destructors of namespace scope objects get registered as
atexit() handlers on some implementations).

[ Post a follow-up to this message ]