Re: svn commit: r1899609 - in /httpd/httpd/trunk: changes-entries/core_ap_escape_quotes.txt server/util.c test/unit/util.c
On Wed, Apr 6, 2022 at 11:17 AM wrote:
>
> Modified: httpd/httpd/trunk/server/util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1899609&r1=1899608&r2=1899609&view=diff
> ==
> --- httpd/httpd/trunk/server/util.c (original)
> +++ httpd/httpd/trunk/server/util.c Wed Apr 6 09:17:42 2022
> @@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower
> */
> AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
> {
> -int newlen = 0;
> +apr_ssize_t extra = 0;
Shouldn't it be an apr_size_t?
> const char *inchr = instring;
> char *outchr, *outstring;
>
> @@ -2624,9 +2624,8 @@ AP_DECLARE(char *) ap_escape_quotes(apr_
> * string up by an extra byte each time we find an unescaped ".
> */
> while (*inchr != '\0') {
> -newlen++;
> if (*inchr == '"') {
> -newlen++;
> +extra++;
> }
> /*
> * If we find a slosh, and it's not the last byte in the string,
> @@ -2634,11 +2633,15 @@ AP_DECLARE(char *) ap_escape_quotes(apr_
> */
> else if ((*inchr == '\\') && (inchr[1] != '\0')) {
> inchr++;
> -newlen++;
> }
> inchr++;
> }
> -outstring = apr_palloc(p, newlen + 1);
> +
> +if (!extra) {
> +return apr_pstrdup(p, instring);
> +}
> +
> +outstring = apr_palloc(p, (inchr - instring) + extra + 1);
> inchr = instring;
> outchr = outstring;
Regards;
Yann.
Re: svn commit: r1899609 - in /httpd/httpd/trunk: changes-entries/core_ap_escape_quotes.txt server/util.c test/unit/util.c
> Am 15.04.2022 um 15:24 schrieb Yann Ylavic :
>
> On Wed, Apr 6, 2022 at 11:17 AM wrote:
>>
>> Modified: httpd/httpd/trunk/server/util.c
>> URL:
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1899609&r1=1899608&r2=1899609&view=diff
>> ==
>> --- httpd/httpd/trunk/server/util.c (original)
>> +++ httpd/httpd/trunk/server/util.c Wed Apr 6 09:17:42 2022
>> @@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower
>> */
>> AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
>> {
>> -int newlen = 0;
>> +apr_ssize_t extra = 0;
>
> Shouldn't it be an apr_size_t?
Similar comment raised on the PR https://github.com/apache/httpd/pull/298
Not totally sure. The thing is that C in general has a problem with
strings where ptrdiff_t (apr_ssize_t) is not sufficient. Allocating something
larger than ptridff_t leads to undefined behaviour.
So, maybe we should check that "(inchr - instring) + extra + 1" does not
wrap around?
>
>> const char *inchr = instring;
>> char *outchr, *outstring;
>>
>> @@ -2624,9 +2624,8 @@ AP_DECLARE(char *) ap_escape_quotes(apr_
>> * string up by an extra byte each time we find an unescaped ".
>> */
>> while (*inchr != '\0') {
>> -newlen++;
>> if (*inchr == '"') {
>> -newlen++;
>> +extra++;
>> }
>> /*
>> * If we find a slosh, and it's not the last byte in the string,
>> @@ -2634,11 +2633,15 @@ AP_DECLARE(char *) ap_escape_quotes(apr_
>> */
>> else if ((*inchr == '\\') && (inchr[1] != '\0')) {
>> inchr++;
>> -newlen++;
>> }
>> inchr++;
>> }
>> -outstring = apr_palloc(p, newlen + 1);
>> +
>> +if (!extra) {
>> +return apr_pstrdup(p, instring);
>> +}
>> +
>> +outstring = apr_palloc(p, (inchr - instring) + extra + 1);
>> inchr = instring;
>> outchr = outstring;
>
>
> Regards;
> Yann.
Re: svn commit: r1899609 - in /httpd/httpd/trunk: changes-entries/core_ap_escape_quotes.txt server/util.c test/unit/util.c
On 4/15/22 4:21 PM, Stefan Eissing wrote:
>
>
>> Am 15.04.2022 um 15:24 schrieb Yann Ylavic :
>>
>> On Wed, Apr 6, 2022 at 11:17 AM wrote:
>>>
>>> Modified: httpd/httpd/trunk/server/util.c
>>> URL:
>>> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1899609&r1=1899608&r2=1899609&view=diff
>>> ==
>>> --- httpd/httpd/trunk/server/util.c (original)
>>> +++ httpd/httpd/trunk/server/util.c Wed Apr 6 09:17:42 2022
>>> @@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower
>>> */
>>> AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
>>> {
>>> -int newlen = 0;
>>> +apr_ssize_t extra = 0;
>>
>> Shouldn't it be an apr_size_t?
>
> Similar comment raised on the PR https://github.com/apache/httpd/pull/298
>
> Not totally sure. The thing is that C in general has a problem with
> strings where ptrdiff_t (apr_ssize_t) is not sufficient. Allocating something
> larger than ptridff_t leads to undefined behaviour.
>
> So, maybe we should check that "(inchr - instring) + extra + 1" does not
> wrap around?
I did some tests with gcc 8.5 and it seems to do the "expected" thing if two
pointers are farther away than PTRDIFF_MAX:
It provides the correct difference. This does not work if the difference would
be negative (just swapped the pointers for the test).
Having said this the above does not mean that other compilers or other versions
of gcc behave the same as the specs for C do not
define a behavior in this case. I guess the best thing we can do is:
1. Make extra apr_size_t
2. Check that inchr - instring remains >= 0 .
3. Store inchr - instring in an apr_size_t.
4. Check that inchr - instring <= SIZE_MAX - extra - 1
5. If 2. and 4. are true calculate (inchr - instring) + extra + 1 otherwise
abort() as at least 4 means we would be OOM anyway and
2. does not offer a sane way to us to solve this.
Regards
Rüdiger
Re: svn commit: r1899609 - in /httpd/httpd/trunk: changes-entries/core_ap_escape_quotes.txt server/util.c test/unit/util.c
On Fri, Apr 15, 2022 at 4:21 PM Stefan Eissing wrote:
>
> > Am 15.04.2022 um 15:24 schrieb Yann Ylavic :
> >
> > On Wed, Apr 6, 2022 at 11:17 AM wrote:
> >>
> >> Modified: httpd/httpd/trunk/server/util.c
> >> URL:
> >> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1899609&r1=1899608&r2=1899609&view=diff
> >> ==
> >> --- httpd/httpd/trunk/server/util.c (original)
> >> +++ httpd/httpd/trunk/server/util.c Wed Apr 6 09:17:42 2022
> >> @@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower
> >> */
> >> AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
> >> {
> >> -int newlen = 0;
> >> +apr_ssize_t extra = 0;
> >
> > Shouldn't it be an apr_size_t?
>
> Similar comment raised on the PR https://github.com/apache/httpd/pull/298
Oh, I missed it.
>
> Not totally sure. The thing is that C in general has a problem with
> strings where ptrdiff_t (apr_ssize_t) is not sufficient. Allocating something
> larger than ptridff_t leads to undefined behaviour.
On 32bit systems, ssize_t = ptrdiff_t = int, I think allocating
something larger than INT_MAX is possible if you have the memory
available for it.
>
> So, maybe we should check that "(inchr - instring) + extra + 1" does not
> wrap around?
Maybe something like:
apr_size_t size, extra = 0;
...
size = inchr - instring + 1;
ap_assert(size + extra > size);
size += extra;
?
Re: svn commit: r1899609 - in /httpd/httpd/trunk: changes-entries/core_ap_escape_quotes.txt server/util.c test/unit/util.c
On Fri, Apr 15, 2022 at 6:19 PM Yann Ylavic wrote:
>
> On Fri, Apr 15, 2022 at 4:21 PM Stefan Eissing wrote:
> >
> > > Am 15.04.2022 um 15:24 schrieb Yann Ylavic :
> > >
> > > On Wed, Apr 6, 2022 at 11:17 AM wrote:
> > >>
> > >> Modified: httpd/httpd/trunk/server/util.c
> > >> URL:
> > >> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1899609&r1=1899608&r2=1899609&view=diff
> > >> ==
> > >> --- httpd/httpd/trunk/server/util.c (original)
> > >> +++ httpd/httpd/trunk/server/util.c Wed Apr 6 09:17:42 2022
> > >> @@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower
> > >> */
> > >> AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
> > >> {
> > >> -int newlen = 0;
> > >> +apr_ssize_t extra = 0;
> > >
> > > Shouldn't it be an apr_size_t?
> >
> > Similar comment raised on the PR https://github.com/apache/httpd/pull/298
>
> Oh, I missed it.
>
> >
> > Not totally sure. The thing is that C in general has a problem with
> > strings where ptrdiff_t (apr_ssize_t) is not sufficient. Allocating
> > something
> > larger than ptridff_t leads to undefined behaviour.
>
> On 32bit systems, ssize_t = ptrdiff_t = int, I think allocating
> something larger than INT_MAX is possible if you have the memory
> available for it.
>
> >
> > So, maybe we should check that "(inchr - instring) + extra + 1" does not
> > wrap around?
>
> Maybe something like:
> apr_size_t size, extra = 0;
> ...
> size = inchr - instring + 1;
> ap_assert(size + extra > size);
Well, ap_assert(size + extra >= size) of course..
> size += extra;
> ?
Re: svn commit: r1899609 - in /httpd/httpd/trunk: changes-entries/core_ap_escape_quotes.txt server/util.c test/unit/util.c
> Am 15.04.2022 um 18:20 schrieb Yann Ylavic :
>
> On Fri, Apr 15, 2022 at 6:19 PM Yann Ylavic wrote:
>>
>> On Fri, Apr 15, 2022 at 4:21 PM Stefan Eissing wrote:
>>>
Am 15.04.2022 um 15:24 schrieb Yann Ylavic :
On Wed, Apr 6, 2022 at 11:17 AM wrote:
>
> Modified: httpd/httpd/trunk/server/util.c
> URL:
> http://svn.apache.org/viewvc/httpd/httpd/trunk/server/util.c?rev=1899609&r1=1899608&r2=1899609&view=diff
> ==
> --- httpd/httpd/trunk/server/util.c (original)
> +++ httpd/httpd/trunk/server/util.c Wed Apr 6 09:17:42 2022
> @@ -2615,7 +2615,7 @@ AP_DECLARE(void) ap_content_type_tolower
> */
> AP_DECLARE(char *) ap_escape_quotes(apr_pool_t *p, const char *instring)
> {
> -int newlen = 0;
> +apr_ssize_t extra = 0;
Shouldn't it be an apr_size_t?
>>>
>>> Similar comment raised on the PR https://github.com/apache/httpd/pull/298
>>
>> Oh, I missed it.
>>
>>>
>>> Not totally sure. The thing is that C in general has a problem with
>>> strings where ptrdiff_t (apr_ssize_t) is not sufficient. Allocating
>>> something
>>> larger than ptridff_t leads to undefined behaviour.
>>
>> On 32bit systems, ssize_t = ptrdiff_t = int, I think allocating
>> something larger than INT_MAX is possible if you have the memory
>> available for it.
>>
>>>
>>> So, maybe we should check that "(inchr - instring) + extra + 1" does not
>>> wrap around?
>>
>> Maybe something like:
>>apr_size_t size, extra = 0;
>>...
>>size = inchr - instring + 1;
>>ap_assert(size + extra > size);
>
> Well, ap_assert(size + extra >= size) of course..
Added in r1899905. Note that the > is sufficient since !extra is checked above.
>
>>size += extra;
>> ?
