Mailing List Archive

Funny you should mention the fgets() in read_line(), I just noticed that this
morning, too! This way my fix:

tmpline = fgets(tmpline, MAX_LINE_SIZE, instream);
+ if (!tmpline)
+ continue;

On the next turn around the while loop, feof(instream) stops the loop.
Although I'm also thinking that this might be inviting trouble in the form of
an infinite loop if there was some other reason for fgets() to return NULL...
what was your solution?

The makemd5() function looks like it's on shaky ground to me. Lots of magic
numbers being used for things... but it's within the md5 functions that the
problem is occurring...

Ok, try this: in makemd5(), change result[16] to be twice as large. Don't
change the size of the malloc(33), since we can use valgrind to try to catch
the memory error to confirm my theory. What I'm thinking is that perhaps the
gdm_md5_final() function is producing a larger result due to being 64 bit, and
smashing the stack by running off the end of result[16].

Aaron


Ilja Booij <ilja@ic-s.nl> said:

> Hi all,
>
> A few days ago Paul F De La Cruz told us that dbmail was crashing on his
> Dual Opteron system. He has given me the opportunity to use his system
> for debugging.
>
> I actually found two bugs. One was a bug that I could reproduce on my
> own systems.
>
> It's located in header.c, in the function read_header(). In line 72,
> fgets can return NULL. There was no check for this, so the strlen() on
> line 73 segfaulted.
>
> After fixing this bug, I happily, and wrongly ;), concluded that I had
> fixed it all! Paul told me that the thing was still segfaulting.. It
> turned out to be the makemd5() function from dbmd5.c that eventually
> (somewhere deep down in the md5 functions) overwrites some memory it
> should not overwrite. The md5 algorithm is pretty unclear to me, so I
> cannot find what is going wrong.
>
> running valgrind on x86 did not reveal any problems.
>
> Does anyone have an idea how to fix this?
>
> Ilja
> _______________________________________________
> Dbmail-dev mailing list
> Dbmail-dev@dbmail.org
> http://twister.fastxs.net/mailman/listinfo/dbmail-dev
>



--

Hi,

Aaron Stone wrote:

> Funny you should mention the fgets() in read_line(), I just noticed that this
> morning, too! This way my fix:
>
> tmpline = fgets(tmpline, MAX_LINE_SIZE, instream);
> + if (!tmpline)
> + continue;
>
> On the next turn around the while loop, feof(instream) stops the loop.
> Although I'm also thinking that this might be inviting trouble in the form of
> an infinite loop if there was some other reason for fgets() to return NULL...
> what was your solution?
tmpline = fgets(tmpline, MAX_LINE_SIZE, instream);
if (!tmpline)
break;

>
> The makemd5() function looks like it's on shaky ground to me. Lots of magic
> numbers being used for things... but it's within the md5 functions that the
> problem is occurring...
>
> Ok, try this: in makemd5(), change result[16] to be twice as large. Don't
> change the size of the malloc(33), since we can use valgrind to try to catch
> the memory error to confirm my theory. What I'm thinking is that perhaps the
> gdm_md5_final() function is producing a larger result due to being 64 bit, and
> smashing the stack by running off the end of result[16].
Doesn't help I'm afraid.

Strange thing though. Without -O2, the error seems to be in md5
functions(), but with -O2, it's in a PQEscapeString (on PostgreSQL of
course)

Ilja
>
> Aaron
>
>
> Ilja Booij <ilja@ic-s.nl> said:
>
>
>>Hi all,
>>
>>A few days ago Paul F De La Cruz told us that dbmail was crashing on his
>>Dual Opteron system. He has given me the opportunity to use his system
>>for debugging.
>>
>>I actually found two bugs. One was a bug that I could reproduce on my
>>own systems.
>>
>>It's located in header.c, in the function read_header(). In line 72,
>>fgets can return NULL. There was no check for this, so the strlen() on
>>line 73 segfaulted.
>>
>>After fixing this bug, I happily, and wrongly ;), concluded that I had
>>fixed it all! Paul told me that the thing was still segfaulting.. It
>>turned out to be the makemd5() function from dbmd5.c that eventually
>>(somewhere deep down in the md5 functions) overwrites some memory it
>>should not overwrite. The md5 algorithm is pretty unclear to me, so I
>>cannot find what is going wrong.
>>
>>running valgrind on x86 did not reveal any problems.
>>
>>Does anyone have an idea how to fix this?
>>
>>Ilja
>>_______________________________________________
>>Dbmail-dev mailing list
>>Dbmail-dev@dbmail.org
>>http://twister.fastxs.net/mailman/listinfo/dbmail-dev
>>
>
>
>
>

Ilja Booij <ilja@ic-s.nl> said:

> Hi,
>
> Aaron Stone wrote:
>
> > Funny you should mention the fgets() in read_line(), I just noticed that
> > this morning, too! This way my fix:
> >
> > tmpline = fgets(tmpline, MAX_LINE_SIZE, instream);
> > + if (!tmpline)
> > + continue;
> >
> > On the next turn around the while loop, feof(instream) stops the loop.
> > Although I'm also thinking that this might be inviting trouble in the
> > form of an infinite loop if there was some other reason for fgets() to
> > return NULL...
> > what was your solution?
> tmpline = fgets(tmpline, MAX_LINE_SIZE, instream);
> if (!tmpline)
> break;
>

Well now, that makes sense :-P

> >
> > The makemd5() function looks like it's on shaky ground to me. Lots of
> > magic numbers being used for things... but it's within the md5 functions
> > that the problem is occurring...
> >
> > Ok, try this: in makemd5(), change result[16] to be twice as large. Don't
> > change the size of the malloc(33), since we can use valgrind to try to
> > catch the memory error to confirm my theory. What I'm thinking is that
> > perhaps the gdm_md5_final() function is producing a larger result due to
> > being 64 bit, and smashing the stack by running off the end of result[16].
> Doesn't help I'm afraid.
>
> Strange thing though. Without -O2, the error seems to be in md5
> functions(), but with -O2, it's in a PQEscapeString (on PostgreSQL of
> course)

In my experience, when a problem moves around due to optimization, it's either
because you're triggering a different segfault somewhere else first, and your
original bug and a new bug have been uncovered... or because there's an
overflow somewhere way out in left field that's randomly screwing things up
just depending upon the layout of the functions in memory :-\

It occurs to me that valgrind actually emulates a Pentium instruction set. I
might be on a wild goose chase with the 64-bit thing, but valgrind should
either be unable to monitor such a program or just freak out about the 64-bits
and the additional x86-64 instructions that it doesn't know about... or
perhaps it just glosses over them like it does with SSE, etc?

>
> Ilja
> >
> > Aaron
> >
> >
> > Ilja Booij <ilja@ic-s.nl> said:
> >
> >
> >>Hi all,
> >>
> >>A few days ago Paul F De La Cruz told us that dbmail was crashing on his
> >>Dual Opteron system. He has given me the opportunity to use his system
> >>for debugging.
> >>
> >>I actually found two bugs. One was a bug that I could reproduce on my
> >>own systems.
> >>
> >>It's located in header.c, in the function read_header(). In line 72,
> >>fgets can return NULL. There was no check for this, so the strlen() on
> >>line 73 segfaulted.
> >>
> >>After fixing this bug, I happily, and wrongly ;), concluded that I had
> >>fixed it all! Paul told me that the thing was still segfaulting.. It
> >>turned out to be the makemd5() function from dbmd5.c that eventually
> >>(somewhere deep down in the md5 functions) overwrites some memory it
> >>should not overwrite. The md5 algorithm is pretty unclear to me, so I
> >>cannot find what is going wrong.
> >>
> >>running valgrind on x86 did not reveal any problems.
> >>
> >>Does anyone have an idea how to fix this?
> >>
> >>Ilja
> >>_______________________________________________
> >>Dbmail-dev mailing list
> >>Dbmail-dev@dbmail.org
> >>http://twister.fastxs.net/mailman/listinfo/dbmail-dev
> >>
> >
> >
> >
> >
> _______________________________________________
> Dbmail-dev mailing list
> Dbmail-dev@dbmail.org
> http://twister.fastxs.net/mailman/listinfo/dbmail-dev
>



--

Actually valgrind refuses to compile on x86_64. I tried to get it installed on the opteron server last night and it just dies on ./configure stating the cpu isn't supported. Really burst my bubble as I got settled in to try debugging dbmail-smtp yesterday. :(

---------- Original Message -------------
Subject: Re: [Dbmail-dev] md5 algorithm
Date: Thu, 12 Feb 2004 17:24:47 -0000
From: "Aaron Stone" <aaron@serendipity.palo-alto.ca.us>

Ilja Booij <ilja@ic-s.nl> said:
> > Ok, try this: in makemd5(), change result[16] to be twice as large. Don't
> > change the size of the malloc(33), since we can use valgrind to try to
> > catch the memory error to confirm my theory. What I'm thinking is that
> > perhaps the gdm_md5_final() function is producing a larger result due to
> > being 64 bit, and smashing the stack by running off the end of result[16].
> Doesn't help I'm afraid.
>
> Strange thing though. Without -O2, the error seems to be in md5
> functions(), but with -O2, it's in a PQEscapeString (on PostgreSQL of
> course)

In my experience, when a problem moves around due to optimization, it's either
because you're triggering a different segfault somewhere else first, and your
original bug and a new bug have been uncovered... or because there's an
overflow somewhere way out in left field that's randomly screwing things up
just depending upon the layout of the functions in memory :-\

It occurs to me that valgrind actually emulates a Pentium instruction set. I
might be on a wild goose chase with the 64-bit thing, but valgrind should
either be unable to monitor such a program or just freak out about the 64-bits
and the additional x86-64 instructions that it doesn't know about... or
perhaps it just glosses over them like it does with SSE, etc?

Actually we're both wrong!

if (!fgets(tmpline, MAX_LINE_SIZE, instream);
break;

Because fgets returns NULL for an empty line, tmpline's memory is lost.

I'm putting together more patches and will post them to the SourceForge site
this evening. BTW - do you mind GNU GCC extensions? I quickly implemented the
db_get_result_...() functions (there's u64, int and bool versions) as macros
like so:


#define db_get_result_int(row, field) \
({ \
char *tmp; \
tmp = db_get_result(row, field); \
( tmp ? atoi(tmp) : 0 ); \
})
#define db_get_result_bool(row, field) \
({ \
char *tmp; \
tmp = db_get_result(row, field); \
( tmp ? ( atoi(tmp) ? 1 : 0 ) : 0 ); \
})
#define db_get_result_u64(row, field) \
({ \
char *tmp; \
tmp = db_get_result(row, field); \
( tmp ? strtoull(tmp, NULL, 10) : 0 ); \
})


({ expression; expression; expression; }) is a GNU extension that takes on the
value of the last expression in the block. Lazy stuff. If you'd rather see
these done as functions, it's two seconds work to move them into db.c

Aaron


Ilja Booij <ilja@ic-s.nl> said: [snipped the whole thing]

> tmpline = fgets(tmpline, MAX_LINE_SIZE, instream);
> if (!tmpline)
> break;


--

Of course, you noticed that it should also really really be...

if (!fgets(tmpline, MAX_LINE_SIZE, instream))
break;

"Aaron Stone" <aaron@serendipity.palo-alto.ca.us> said:

> Actually we're both wrong!
>
> if (!fgets(tmpline, MAX_LINE_SIZE, instream);
> break;
>
> Because fgets returns NULL for an empty line, tmpline's memory is lost.
>
> I'm putting together more patches and will post them to the SourceForge site
> this evening. BTW - do you mind GNU GCC extensions? I quickly implemented the
> db_get_result_...() functions (there's u64, int and bool versions) as macros
> like so:
>
>
> #define db_get_result_int(row, field) \
> ({ \
> char *tmp; \
> tmp = db_get_result(row, field); \
> ( tmp ? atoi(tmp) : 0 ); \
> })
> #define db_get_result_bool(row, field) \
> ({ \
> char *tmp; \
> tmp = db_get_result(row, field); \
> ( tmp ? ( atoi(tmp) ? 1 : 0 ) : 0 ); \
> })
> #define db_get_result_u64(row, field) \
> ({ \
> char *tmp; \
> tmp = db_get_result(row, field); \
> ( tmp ? strtoull(tmp, NULL, 10) : 0 ); \
> })
>
>
> ({ expression; expression; expression; }) is a GNU extension that takes on the
> value of the last expression in the block. Lazy stuff. If you'd rather see
> these done as functions, it's two seconds work to move them into db.c
>
> Aaron
>
>
> Ilja Booij <ilja@ic-s.nl> said: [snipped the whole thing]
>
> > tmpline = fgets(tmpline, MAX_LINE_SIZE, instream);
> > if (!tmpline)
> > break;
>
>
> --
> _______________________________________________
> Dbmail-dev mailing list
> Dbmail-dev@dbmail.org
> http://twister.fastxs.net/mailman/listinfo/dbmail-dev
>



--

Hi

Aaron Stone wrote:

> Actually we're both wrong!
>
> if (!fgets(tmpline, MAX_LINE_SIZE, instream);
> break;
>
> Because fgets returns NULL for an empty line, tmpline's memory is lost.
So we should also check if tmpline is NULL, and if it isn't, free it.
(or am I missing something here?)
>
> I'm putting together more patches and will post them to the SourceForge site
> this evening. BTW - do you mind GNU GCC extensions? I quickly implemented the
> db_get_result_...() functions (there's u64, int and bool versions) as macros
> like so:
I guess (almost) everyone is using GCC to compile DBMail, so it
shouldn't really be a problem. It might be cleaner to use functions
though, even if it's only for making code more readable, don't you think?
>
>
> #define db_get_result_int(row, field) \
> ({ \
> char *tmp; \
> tmp = db_get_result(row, field); \
> ( tmp ? atoi(tmp) : 0 ); \
> })
> #define db_get_result_bool(row, field) \
> ({ \
> char *tmp; \
> tmp = db_get_result(row, field); \
> ( tmp ? ( atoi(tmp) ? 1 : 0 ) : 0 ); \
> })
> #define db_get_result_u64(row, field) \
> ({ \
> char *tmp; \
> tmp = db_get_result(row, field); \
> ( tmp ? strtoull(tmp, NULL, 10) : 0 ); \
> })
>
>
> ({ expression; expression; expression; }) is a GNU extension that takes on the
> value of the last expression in the block. Lazy stuff. If you'd rather see
> these done as functions, it's two seconds work to move them into db.c
I'm not that familiar with GCC macros (mostly just use them for
constants), but this kind of macro looks like some real magic stuff :).

I guess we should make these macros into functions. Those are cleaner,
and easier to debug.

Ilja
>
>
>>tmpline = fgets(tmpline, MAX_LINE_SIZE, instream);
>>if (!tmpline)
>> break;
>
>
>
> --
> _______________________________________________
> Dbmail-dev mailing list
> Dbmail-dev@dbmail.org
> http://twister.fastxs.net/mailman/listinfo/dbmail-dev

I've used valgrind on my local box. I tried to find the error we're
looking for using valgrind. I was kind of hoping that valgrind was able
to find the error on x86. I've used valgrind in the past to find errors
that were expressed on one platform (were valgrind did not run), but did
not cause crashes on the 'valgrind-platform'. Valgrind was still able to
find the errors.

Alas, not this time..

I've been looking for a replacement for our md5 functions. We could of
course use OpenSSL's md5 (or even better, SHA1) functions. But this
would mean that we have a dependency on OpenSSL. It does give us the
opportunity to throw away some code (we could get rid of the md5.c and
md5.h files) that is not understood by most people (including me).

I remember something about the GPL not being compatible with OpenSSL's
license. Does anybody know if this also applies to linking?

Ilja

Paul F. De La Cruz wrote:

> Actually valgrind refuses to compile on x86_64. I tried to get it installed on the opteron server last night and it just dies on ./configure stating the cpu isn't supported. Really burst my bubble as I got settled in to try debugging dbmail-smtp yesterday. :(
>
> ---------- Original Message -------------
> Subject: Re: [Dbmail-dev] md5 algorithm
> Date: Thu, 12 Feb 2004 17:24:47 -0000
> From: "Aaron Stone" <aaron@serendipity.palo-alto.ca.us>
>
> Ilja Booij <ilja@ic-s.nl> said:
>
>>>Ok, try this: in makemd5(), change result[16] to be twice as large. Don't
>>>change the size of the malloc(33), since we can use valgrind to try to
>>>catch the memory error to confirm my theory. What I'm thinking is that
>>>perhaps the gdm_md5_final() function is producing a larger result due to
>>>being 64 bit, and smashing the stack by running off the end of result[16].
>>
>>Doesn't help I'm afraid.
>>
>>Strange thing though. Without -O2, the error seems to be in md5
>>functions(), but with -O2, it's in a PQEscapeString (on PostgreSQL of
>>course)
>
>
> In my experience, when a problem moves around due to optimization, it's either
> because you're triggering a different segfault somewhere else first, and your
> original bug and a new bug have been uncovered... or because there's an
> overflow somewhere way out in left field that's randomly screwing things up
> just depending upon the layout of the functions in memory :-\
>
> It occurs to me that valgrind actually emulates a Pentium instruction set. I
> might be on a wild goose chase with the 64-bit thing, but valgrind should
> either be unable to monitor such a program or just freak out about the 64-bits
> and the additional x86-64 instructions that it doesn't know about... or
> perhaps it just glosses over them like it does with SSE, etc?
> _______________________________________________
> Dbmail-dev mailing list
> Dbmail-dev@dbmail.org
> http://twister.fastxs.net/mailman/listinfo/dbmail-dev