Re: cvs commit: apr/strings apr_strings.c
David Reid wrote:
[EMAIL PROTECTED] wrote:
wrowe 2004/06/28 11:09:16
Modified:strings Tag: APR_0_9_BRANCH apr_strings.c
Log:
Avoid any edge case or clib bug that might result in a string
overflow of the fixed 5-byte buffer for our size function.
Returns the '' string when the buffer would overflow.
Backport of rev 1.47
Reviewed by: trawick
reviewed but not approved ;)
it still has the same bug (apr_snprintf() doesn't return 0 either)
Care to supply a fixed version?
IMHO the original version is sufficient for now. In fact, the new version
isn't going to misbehave; it just introduces dead code.
One way of implementing the spirit of what Bill Rowe was trying to do would
require looking at the generated buffer instead of the return code from
sprintf/apr_snprintf.
For example, change
sprintf(buf, %3d , (int) size);
to
apr_snprintf(buf, 5, %3d , (int) size);
if (buf[3] != ' ') { /* catch overflow */
return strcpy(buf, );
}
(For the other changes we'd have to look for the final 'c' in a certain
position to detect overflow.)
Alternatively, apr_snprintf into a larger local buffer to be able to detect
overflow of a 5-byte buffer by looking at the apr_snprintf() return code, and
if okay copy the output to the caller's buffer before returning.
Note that libc has to have broken sprintf() or somebody has to introduce a new
bug into the apr_strfsize() function in order to have such an overflow anyway.
Due to the API, we can't catch the problem where the caller passes a buffer
which is not large enough.
Re: cvs commit: apr/strings apr_strings.c
On Wed, Jun 30, 2004 at 06:00:29AM -0400, Jeff Trawick wrote: IMHO the original version is sufficient for now. Agreed. .. Note that libc has to have broken sprintf() or somebody has to introduce a new bug into the apr_strfsize() function in order to have such an overflow anyway. Due to the API, we can't catch the problem where the caller passes a buffer which is not large enough. And the person who screws up the code in the future may also screw up and remove the snprintf calls when they work out they are redundant. Adding tests seems like the best way to protect against someone screwing up in the future... joe
Re: cvs commit: apr/strings apr_strings.c
Joe Orton wrote: On Wed, Jun 30, 2004 at 06:00:29AM -0400, Jeff Trawick wrote: IMHO the original version is sufficient for now. Agreed. Adding tests seems like the best way to protect against someone screwing up in the future... yes, that makes perfect sense to me
Re: cvs commit: apr/strings apr_strings.c
Jeff Trawick wrote:
reviewed but not approved ;)
it still has the same bug (apr_snprintf() doesn't return 0 either)
Care to supply a fixed version?
IMHO the original version is sufficient for now. In fact, the new version
isn't going to misbehave; it just introduces dead code.
One way of implementing the spirit of what Bill Rowe was trying to do would
require looking at the generated buffer instead of the return code from
sprintf/apr_snprintf.
For example, change
sprintf(buf, %3d , (int) size);
to
apr_snprintf(buf, 5, %3d , (int) size);
if (buf[3] != ' ') { /* catch overflow */
return strcpy(buf, );
}
If I understand the problem correctly, doesn't snprintf() return
the number of bytes that would have been printed if there had been
no limit. Thus, can't we check that the return value is =
the actual buffer size?
--
===
Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/
A society that will trade a little liberty for a little order
will lose both and deserve neither - T.Jefferson
Re: cvs commit: apr/strings apr_strings.c
apr_snprintf(buf, 5, %3d , (int) size);
if (buf[3] != ' ') { /* catch overflow */
return strcpy(buf, );
}
If I understand the problem correctly, doesn't snprintf() return
the number of bytes that would have been printed if there had been
no limit. Thus, can't we check that the return value is =
the actual buffer size?
And apr_snprintf() tells us the same if len is 0.
Re: cvs commit: apr/strings apr_strings.c
Jim Jagielski wrote:
apr_snprintf(buf, 5, %3d , (int) size);
if (buf[3] != ' ') { /* catch overflow */
return strcpy(buf, );
}
If I understand the problem correctly, doesn't snprintf() return
the number of bytes that would have been printed if there had been
no limit. Thus, can't we check that the return value is =
the actual buffer size?
snprintf() is not as portable as we would like, so we need to use
apr_snprintf()
And apr_snprintf() tells us the same if len is 0.
so we'd have to call it twice...
I like Joe's suggestion of catching it in the test suite. If the API is ever
changed so that the caller specifies the length of their buffer, then there
will be an interesting case which apr_snprintf() could catch.
Re: cvs commit: apr/strings apr_strings.c
At 08:17 AM 6/30/2004, Jeff Trawick wrote: I like Joe's suggestion of catching it in the test suite. If the API is ever changed so that the caller specifies the length of their buffer, then there will be an interesting case which apr_snprintf() could catch. Unfortunately, you would need to test the full range of possible inputs or you won't catch an edge case. One question, perhaps, is why we silently succeed while truncating the output string in apr_snprintf(). Obviously some snprintf's pass, some fail, so there must be some religious argument over 'proper' behavior. My personal philosophy, retval -1 for apr_snprintf() would tell the user we succeeded in filling their buffer, and then choked. They gave us the size of the buffer, so they should know how much was filled if they are happy with the truncated result Because the (new) flavor can never overflow (invoking apr_snprintf), I'd be happy ignoring the retval entirely. If the last position is non-alpha, the user knows something was borked. Either way, there isn't the risk of an overflow anymore. Bill
Re: cvs commit: apr/strings apr_strings.c
[EMAIL PROTECTED] wrote:
wrowe 2004/06/28 11:09:16
Modified:strings Tag: APR_0_9_BRANCH apr_strings.c
Log:
Avoid any edge case or clib bug that might result in a string
overflow of the fixed 5-byte buffer for our size function.
Returns the '' string when the buffer would overflow.
Backport of rev 1.47
Reviewed by: trawick
reviewed but not approved ;)
it still has the same bug (apr_snprintf() doesn't return 0 either)
Index: apr_strings.c
===
RCS file: /home/cvs/apr/strings/apr_strings.c,v
retrieving revision 1.42.2.2
retrieving revision 1.42.2.3
diff -u -r1.42.2.2 -r1.42.2.3
--- apr_strings.c 4 Apr 2004 15:21:08 - 1.42.2.2
+++ apr_strings.c 28 Jun 2004 18:09:16 - 1.42.2.3
@@ -429,7 +429,8 @@
return strcpy(buf, - );
}
if (size 973) {
-sprintf(buf, %3d , (int) size);
+if (apr_snprintf(buf, 5, %3d , (int) size) 0)
does not occur... apr_snprintf would return the number of bytes stored in case
of an overflow
Re: cvs commit: apr/strings apr_strings.c
=?UTF-8?B?QnJhbmtvIMSMaWJlag==?= wrote: Aaron Bannert wrote: +if ( (any 0) || (neg (val acc || (val -= c) acc)) Isn't that one of those short-circuits that causes bad side-effects under different circumstances? I have a feeling that it is not well defined if the -= operation is always performed or not, but I could be wrong. It seems well defined to me, but it's a total pain in the nethers to read and understand. Assuming I can understand what's going on here, I'll take a shot at trying to make the logic more clear (while keeping as much of the constant-folding/short-circuiting as possible.) Go go go! Actually, once any is negative, we don't need to bother with any other checks (since we've already noted an overflow). All the above checks do is make sure that we haven't overflowed/under- flowed. But if we already have, then those checks are meaningless and useless and don't need to be done (including the arithmetics) since we don't use 'acc' at all in that case. More a formatting issue than anything. :) -- === Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ A society that will trade a little liberty for a little order will lose both and deserve neither - T.Jefferson
Re: cvs commit: apr/strings apr_strings.c
+if ( (any 0) || (neg (val acc || (val -= c) acc)) Isn't that one of those short-circuits that causes bad side-effects under different circumstances? I have a feeling that it is not well defined if the -= operation is always performed or not, but I could be wrong. Assuming I can understand what's going on here, I'll take a shot at trying to make the logic more clear (while keeping as much of the constant-folding/short-circuiting as possible.) -aaron
Re: cvs commit: apr/strings apr_strings.c
Aaron Bannert wrote: +if ( (any 0) || (neg (val acc || (val -= c) acc)) Isn't that one of those short-circuits that causes bad side-effects under different circumstances? I have a feeling that it is not well defined if the -= operation is always performed or not, but I could be wrong. It seems well defined to me, but it's a total pain in the nethers to read and understand. Assuming I can understand what's going on here, I'll take a shot at trying to make the logic more clear (while keeping as much of the constant-folding/short-circuiting as possible.) Go go go! -- Brane ibej [EMAIL PROTECTED] http://www.xbc.nu/brane/
Re: cvs commit: apr/strings apr_strings.c
[EMAIL PROTECTED] writes: wsanchez2002/07/24 13:29:38 Modified:.CHANGES configure.in include apr.h.in apr_strings.h strings apr_strings.c Log: Added apr_strtoll() and apr_atoll() to strings lib. Submitted by: Shantonu Sen [EMAIL PROTECTED] ... Index: apr_strings.h === RCS file: /home/cvs/apr/include/apr_strings.h,v retrieving revision 1.26 retrieving revision 1.27 diff -u -r1.26 -r1.27 --- apr_strings.h 28 Jun 2002 21:05:14 - 1.26 +++ apr_strings.h 24 Jul 2002 20:29:38 - 1.27 @@ -327,6 +327,30 @@ ... +APR_DECLARE(long long) apr_strtoll(char *buf, char **end, int base); needs to be const char *buf apr_atoll() needs to take const char *buf too -- Jeff Trawick | [EMAIL PROTECTED] Born in Roswell... married an alien...
Re: cvs commit: apr/strings apr_strings.c
This just broke the Windows build. error C2632: 'long' followed by 'long' is illegal Shane [EMAIL PROTECTED] wrote: wsanchez2002/07/24 13:29:38 Modified:.CHANGES configure.in include apr.h.in apr_strings.h strings apr_strings.c Log: Added apr_strtoll() and apr_atoll() to strings lib. Submitted by: Shantonu Sen [EMAIL PROTECTED] Revision ChangesPath 1.311 +3 -0 apr/CHANGES
Re: cvs commit: apr/strings apr_strings.c
[EMAIL PROTECTED] wrote: wsanchez2002/07/24 13:29:38 Modified:.CHANGES configure.in include apr.h.in apr_strings.h strings apr_strings.c Log: Added apr_strtoll() and apr_atoll() to strings lib. Submitted by: Shantonu Sen [EMAIL PROTECTED] You should use apr_int64_t instead of long long. -- Brane ibej [EMAIL PROTECTED] http://www.xbc.nu/brane/
Re: cvs commit: apr/strings apr_strings.c
On 28 Sep 2001 [EMAIL PROTECTED] wrote:
+nargs = 0;
while ((argp = va_arg(adummy, char *)) != NULL) {
len = strlen(argp);
Urm, didnjya mean to remove that last line?
+if (nargs MAX_SAVED_LENGTHS) {
+len = saved_lengths[nargs++];
+}
+else {
+len = strlen(argp);
+}
+
memcpy(cp, argp, len);
cp += len;
}
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: apr/strings apr_strings.c
Hmmm. Actually, this could improve its efficiency by only allocating
len+1 bytes if len n. Should we do that?
Cheers,
Ben.
[EMAIL PROTECTED] wrote:
ben 01/02/11 08:25:08
Modified:strings apr_strings.c
Log:
ap_pstrndup could have caused out-of-bounds memory accesses (this is a
theoretical problem that I happened to notice). Only lightly tested.
Revision ChangesPath
1.9 +7 -2 apr/strings/apr_strings.c
Index: apr_strings.c
===
RCS file: /home/cvs/apr/strings/apr_strings.c,v
retrieving revision 1.8
retrieving revision 1.9
diff -u -r1.8 -r1.9
--- apr_strings.c 2001/02/11 16:18:09 1.8
+++ apr_strings.c 2001/02/11 16:25:07 1.9
@@ -83,13 +83,18 @@
APR_DECLARE(char *) apr_pstrndup(apr_pool_t *a, const char *s, apr_size_t
n)
{
char *res;
+size_t len;
if (s == NULL) {
return NULL;
}
res = apr_palloc(a, n + 1);
-memcpy(res, s, n);
-res[n] = '\0';
+len = strlen(s);
+if(len n) {
+ memcpy(res, s, n);
+ res[n] = '\0';
+} else
+ memcpy(res, s, len+1);
return res;
}
--
http://www.apache-ssl.org/ben.html
There is no limit to what a man can do or how far he can go if he
doesn't mind who gets the credit. - Robert Woodruff
Re: cvs commit: apr/strings apr_strings.c
--- [EMAIL PROTECTED] wrote:
+APR_DECLARE(void *) apr_memdup(apr_pool_t *a, const void *m, apr_size_t n)
+{
+void *res;
+
+if(m == NULL)
+ return NULL;
+res = apr_palloc(a, n);
+memcpy(res, m, n);
+return res;
+}
+
Nice. Minor question, though: why is this called apr_memdup instead of
apr_pmemdup?
I thought all of the like the standard function, but into a pool instead of
the heap
functions were prefixed with a 'p'.
Also, should it check for failure of apr_palloc, or is that being too anal?
--Cliff
__
Do You Yahoo!?
Get personalized email addresses from Yahoo! Mail - only $35
a year! http://personal.mail.yahoo.com/
