HPCC-30942 Fix SOAPCALL handling of lowercase HTTP headers in response #18095

afishbeck · 2023-11-29T23:51:47Z

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

afishbeck · 2023-11-29T23:52:54Z

@ghalliday draft PR hasn't been tested yet. For your initial review.. and to make sure compiles on windows, etc.

afishbeck · 2023-11-29T23:53:48Z

common/thorhelper/thorsoapcall.cpp

        return true;
-    if (strstr(httpheaders, match))
+    if (strcasestr(httpheaders, match))


@ghalliday not sure if there is JLIB alternative to strcasestr()?

github-actions · 2023-11-29T23:57:54Z

https://track.hpccsystems.com/browse/HPCC-30942
Jira updated

Signed-off-by: Anthony Fishbeck <[email protected]>

afishbeck · 2023-11-30T23:07:39Z

system/jlib/jstring.cpp

+ *               07/04/95  Bob Stout         ANSI-fy
+ *               02/03/94  Fred Cole         Original
+ */
+


@ghalliday strcasestr() didn't compile on windows-22. Tweaked a public domain version of stristr(). Let me know if you'd prefer another approach.

afishbeck · 2023-11-30T23:08:12Z

@ghalliday not sure which release you'd like to target for this.

ghalliday

@afishbeck I'm not sure what I think. It is probably the minimal change to fix the bug, but I had been wondering about the implementation of the current functions. (Only spotted because I have been looking at this code recently.)
The stristr function is a good idea, but I think the implementation could be improved. It could also do with some unit tests (along with everything else in the system!)

So we could take it, and then improve/fix the functions as a separate PR, or we could improve them and fix this at the same time.
What are your thoughts?

ghalliday · 2023-12-01T11:49:56Z

system/jlib/jstring.cpp

+                return nullptr;
+        }
+
+        sptr = start;


move the definition of sptr here (delete line 2874):

const char * sptr = start;

ghalliday · 2023-12-01T11:50:45Z

system/jlib/jstring.cpp

+        }
+
+        sptr = start;
+        pptr = (char *) needle;


Move the definition of pptr here, and remove the cast.

const char * pptr = needle;

Right. Originally they weren't const strings but I changed it and forgot this one.

ghalliday · 2023-12-01T11:51:26Z

system/jlib/jstring.cpp

+    const char *pptr  = needle;     /* Pattern to search for    */
+    const char *start = haystack;   /* Start with a bowl of hay */
+    const char *sptr;               /* Substring pointer        */
+    int   slen  = strlen(haystack); /* Total size of haystack   */


slen, plen should really be size_t, not int.

ghalliday · 2023-12-01T11:59:27Z

system/jlib/jstring.cpp

+    const char *start = haystack;   /* Start with a bowl of hay */
+    const char *sptr;               /* Substring pointer        */
+    int   slen  = strlen(haystack); /* Total size of haystack   */
+    int   plen  = strlen(needle);   /* Length of our needle     */


Should be nlen!

Yes, I think the p was for pattern. But confusing, and really a prefix of "p" for anything but pointer can be confusing.

ghalliday · 2023-12-01T12:00:18Z

system/jlib/jstring.cpp

+    /* while string length not shorter than pattern length */
+    for (; slen >= plen; start++, slen--)
+    {
+        /* find start of pattern in string */


I think I would have coded as (untested):

char needle0 := tolower(*needle); for (; slen >= plen; start++, slen--) { if (tolower(*start) != needle0) { size_t i = 1; for (;;) { if (i == plen) return start; if (tolower(start[i] != tolower(needle[i])) break; i++; } } } return nullptr;

It depends how important efficiency is (and how clever the c++ compiler is).

ghalliday · 2023-12-01T12:07:57Z

common/thorhelper/thorsoapcall.cpp

@@ -1609,9 +1609,9 @@ bool httpHeaderBlockContainsHeader(const char *httpheaders, const char *header)
        return false;
    VStringBuffer match("\n%s:", header);


I did wonder about recoding this, to walk through checking for matches and then skipping to the next \n. It would avoid creating a temporary string, and would (debatebly) probably be more efficient.

If I did do that I would implement a
const char * findHttpHeaderValue(const char *httpheaders, const char *header)
function that returned a pointer to the value, which could then be used in the httpHeaderBlockContainsHeader and getHTTPHeader functions.

ghalliday · 2023-12-01T12:09:37Z

common/thorhelper/thorsoapcall.cpp

@@ -1621,7 +1621,7 @@ bool getHTTPHeader(const char *httpheaders, const char *header, StringBuffer& va
    if (!httpheaders || !*httpheaders || !header || !*header)
        return false;

-    const char* pHeader = strstr(httpheaders, header);
+    const char* pHeader = stristr(httpheaders, header);


This function doesn't have the same checks as the one above. In particular,

"content-length" would match "\nxcontent-length: 100\n"

ghalliday · 2023-12-01T12:10:51Z

common/thorhelper/thorsoapcall.cpp

@@ -1947,10 +1947,10 @@ class CWSCAsyncFor : implements IWSCAsyncFor, public CInterface, public CAsyncFo
                    s = strstr(buffer, " ");
                    if (s)
                        rval = atoi(s+1);
-                    if (!strstr(buffer,"Transfer-Encoding: chunked"))
+                    if (!stristr(buffer,"Transfer-Encoding: chunked"))


These should also really be using getHTTPHeader(dbgheader).

afishbeck · 2023-12-01T16:52:16Z

@afishbeck I'm not sure what I think. It is probably the minimal change to fix the bug, but I had been wondering about the implementation of the current functions. (Only spotted because I have been looking at this code recently.) The stristr function is a good idea, but I think the implementation could be improved. It could also do with some unit tests (along with everything else in the system!)

So we could take it, and then improve/fix the functions as a separate PR, or we could improve them and fix this at the same time. What are your thoughts?

@ghalliday I definitely noticed the code could use improvement. I was trying to minimize the fix to support HAProxy 2.0 thinking the change would likely go into an older branch (not sure how far back). If it were targeting latest I would definitely recommend improving the code right away for this fix I'm not sure. I can add the low risk improvements and open a jira for further refactoring?

ghalliday · 2023-12-07T17:19:50Z

I will merge as-is, but have created a jira to go back and clean it up.

afishbeck commented Nov 29, 2023

View reviewed changes

HPCC-30942 Fix SOAPCALL handling of lowercase HTTP headers in response

9c44801

Signed-off-by: Anthony Fishbeck <[email protected]>

afishbeck force-pushed the soapcallCaseInsentiveHTTPResponse branch from f2ee204 to 9c44801 Compare November 30, 2023 23:05

afishbeck marked this pull request as ready for review November 30, 2023 23:05

afishbeck commented Nov 30, 2023

View reviewed changes

afishbeck requested a review from ghalliday November 30, 2023 23:07

ghalliday reviewed Dec 1, 2023

View reviewed changes

ghalliday merged commit 75bbc4e into hpcc-systems:candidate-9.2.x Dec 7, 2023
48 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-30942 Fix SOAPCALL handling of lowercase HTTP headers in response #18095

HPCC-30942 Fix SOAPCALL handling of lowercase HTTP headers in response #18095

afishbeck commented Nov 29, 2023 •

edited

Loading

afishbeck commented Nov 29, 2023

afishbeck Nov 29, 2023

github-actions bot commented Nov 29, 2023

afishbeck Nov 30, 2023

afishbeck commented Nov 30, 2023

ghalliday left a comment

ghalliday Dec 1, 2023

ghalliday Dec 1, 2023

afishbeck Dec 1, 2023

ghalliday Dec 1, 2023

ghalliday Dec 1, 2023

afishbeck Dec 1, 2023

ghalliday Dec 1, 2023

ghalliday Dec 1, 2023

ghalliday Dec 1, 2023

ghalliday Dec 1, 2023

ghalliday Dec 1, 2023

afishbeck commented Dec 1, 2023

ghalliday commented Dec 7, 2023

		@@ -1609,9 +1609,9 @@ bool httpHeaderBlockContainsHeader(const char httpheaders, const char header)
		return false;
		VStringBuffer match("\n%s:", header);

HPCC-30942 Fix SOAPCALL handling of lowercase HTTP headers in response #18095

HPCC-30942 Fix SOAPCALL handling of lowercase HTTP headers in response #18095

Conversation

afishbeck commented Nov 29, 2023 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

afishbeck commented Nov 29, 2023

Choose a reason for hiding this comment

github-actions bot commented Nov 29, 2023

Choose a reason for hiding this comment

afishbeck commented Nov 30, 2023

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afishbeck commented Dec 1, 2023

ghalliday commented Dec 7, 2023

afishbeck commented Nov 29, 2023 •

edited

Loading