patch file attached. Subitems:
------------------------- {
+#if 0 /* [i_a] ERROR: some intrinsic memcpy() ops can copy DOWN, thus
corrupting the data here */
memcpy((unsigned char *)ctx->tmp,
(unsigned char *)&(ctx->tmp[jj]),i-jj);
+#else
+ memmove(ctx->tmp, &ctx->tmp[jj], i-jj);
+#endif
ctx->tmp_len=i-jj;
Some platforms (MSVC200x with intrinsic functions and maximum speed
optimizations, for example) will produce a memcpy() which does not copy
/upwards/ but rather /downwards/ (counting indexes down to zero, so to
speak). Perfectly legal with respect to the standards; a bit of a bother in
this code as it will blow your desirable data to smithereens. Ouch.
-----------------------------------
static int b64_write(BIO *b, const char *in, int inl)
{
- int ret=inl,n,i;
+ int ret=0; /* [i_a] ERROR: (int)inl; will cause race condition for
nonblock bio returning retry code to us below */
+ int n;
+ int i;
BIO_B64_CTX *ctx;
was: ret=inl; as a starter;
is: ret=0; as a starter, then add each piece we processed to ret.
Sorry, 'race' is a misnomer; my English isn't always up to snuff 24/7.
The point is:
imagine the BIO_write()s in there (in the loop), being stacked on top of
another BIO, and everybody happily running in NONBLOCKING mode, then /this/
code may receive 'retry' error codes from those BIO_write()s due to the
nonblocky nature of the BIO chain then.
Old code will then run with ret==inl i.e. 'everything done! You can all go
back to sleep now!' which is VERY WRONG when the retry happens halfway
through.
Result:
Severe loss of data; corrupted stream. You're OUT.
How did that happen?
Have another look at those lines
i=BIO_write(b->next_bio,&(ctx->buf[ctx->buf_off]),n);
if (i <= 0)
{
BIO_copy_next_retry(b);
return((ret == 0)?i:ret);
}
especially the return statement there and given the old 'ret=inl' init,
think it through.
It is much healthier to the BIO chain and caller to report the /actual/
number of bytes /processed/ than just forgetting about the remainder of the
b64 write buffer when a /retry/ signal occurs. Hence the fix:
ret=0;
while (loop)
{
do_thy_thing(chunk);
ret += chunk;
}
The OPENSSL_assert() plethora in there has been part of the debugging effort
back then (this has been an old bug, which I never got to filing before :-((
); you may want to keep them as there's additional shite happening when
someone mistakes this filter BIO for a 'bidirectional' one. But that's a
whole another diff (independent read and write channel b64 enc/dec)
------------------------- {
Rest of path is a typo fix, introduction of the puts() member for this BIO
(so one can write text strings using BIO_puts() to the BIO chain) and the
if ((ctx == NULL) || (b->next_bio == NULL)) return(0);
+ BIO_clear_retry_flags(b);
+
if (ctx->encode != B64_DECODE)
vs.
outl-=i;
out+=i;
}
- BIO_clear_retry_flags(b);
+ /* BIO_clear_retry_flags(b); */
BIO_copy_next_retry(b);
return((ret == 0)?ret_code:ret);
}
is a fix for the retry flag logic, which, if I'm not mistaken, should always
look like this:
read/write()
{
BIO_clear_retry_flags(b);
do_work() / call_next_BIO_in_chain();
BIO_copy_next_retry(b);
return ret;
}
instead of what a (very) few BIOs (like this one) have:
read/write()
{
do_work() / call_next_BIO_in_chain();
BIO_clear_retry_flags(b);
BIO_copy_next_retry(b);
return ret;
}
as the latter will loose any retry flagging done in the do_work() section of
the implementation.
--
Met vriendelijke groeten / Best regards,
Ger Hobbelt
--------------------------------------------------
web: http://www.hobbelt.com/
http://www.hebbut.net/
mail: g...@hobbelt.com
mobile: +31-6-11 120 978
--------------------------------------------------
patch file attached.Subitems:
------------------------- {
+#if 0 /* [i_a] ERROR: some intrinsic memcpy() ops can copy DOWN, thus corrupting the data here */
memcpy((unsigned char *)ctx->tmp,
(unsigned char *)&(ctx->tmp[jj]),i-jj);
+#else
+ memmove(ctx->tmp, &ctx->tmp[jj], i-jj);
+#endif
ctx->tmp_len=i-jj;
Some platforms (MSVC200x with intrinsic functions and maximum speed optimizations, for example) will produce a memcpy() which does not copy /upwards/ but rather /downwards/ (counting indexes down to zero, so to speak). Perfectly legal with respect to the standards; a bit of a bother in this code as it will blow your desirable data to smithereens. Ouch.
-----------------------------------
static int b64_write(BIO *b, const char *in, int inl)
{
- int ret=inl,n,i;
+ int ret=0; /* [i_a] ERROR: (int)inl; will cause race condition for nonblock bio returning retry code to us below */
+ int n;
+ int i;
BIO_B64_CTX *ctx;
was: ret=inl; as a starter;
is: ret=0; as a starter, then add each piece we processed to ret.
Sorry, 'race' is a misnomer; my English isn't always up to snuff 24/7.
The point is:
imagine the BIO_write()s in there (in the loop), being stacked on top of another BIO, and everybody happily running in NONBLOCKING mode, then /this/ code may receive 'retry' error codes from those BIO_write()s due to the nonblocky nature of the BIO chain then.
Old code will then run with ret==inl i.e. 'everything done! You can all go back to sleep now!' which is VERY WRONG when the retry happens halfway through.
Result:
Severe loss of data; corrupted stream. You're OUT.
How did that happen?
Have another look at those lines
i=BIO_write(b->next_bio,&(ctx->buf[ctx->buf_off]),n);
if (i <= 0)
{
BIO_copy_next_retry(b);
return((ret == 0)?i:ret);
}
especially the return statement there and given the old 'ret=inl' init, think it through.
It is much healthier to the BIO chain and caller to report the /actual/ number of bytes /processed/ than just forgetting about the remainder of the b64 write buffer when a /retry/ signal occurs. Hence the fix:
ret=0;
while (loop)
{
do_thy_thing(chunk);
ret += chunk;
}
The OPENSSL_assert() plethora in there has been part of the debugging effort back then (this has been an old bug, which I never got to filing before :-(( ); you may want to keep them as there's additional shite happening when someone mistakes this filter BIO for a 'bidirectional' one. But that's a whole another diff (independent read and write channel b64 enc/dec)
------------------------- {
Rest of path is a typo fix, introduction of the puts() member for this BIO (so one can write text strings using BIO_puts() to the BIO chain) and the
if ((ctx == NULL) || (b->next_bio == NULL)) return(0);
+ BIO_clear_retry_flags(b);
+
if (ctx->encode != B64_DECODE)
vs.
outl-=i;
out+=i;
}
- BIO_clear_retry_flags(b);
+ /* BIO_clear_retry_flags(b); */
BIO_copy_next_retry(b);
return((ret == 0)?ret_code:ret);
}
is a fix for the retry flag logic, which, if I'm not mistaken, should always look like this:
read/write()
{
BIO_clear_retry_flags(b);
do_work() / call_next_BIO_in_chain();
BIO_copy_next_retry(b);
return ret;
}
instead of what a (very) few BIOs (like this one) have:
read/write()
{
do_work() / call_next_BIO_in_chain();
BIO_clear_retry_flags(b);
BIO_copy_next_retry(b);
return ret;
}
as the latter will loose any retry flagging done in the do_work() section of the implementation.
--
Met vriendelijke groeten / Best regards,
Ger Hobbelt
--------------------------------------------------
web: http://www.hobbelt.com/
http://www.hebbut.net/
mail: g...@hobbelt.com
mobile: +31-6-11 120 978
--------------------------------------------------
evp_bio_b64_c_bug1_2_feat1.diff
Description: Binary data
