Closed Bug 377049 Opened 15 years ago Closed 15 years ago

nsBaseURLParser::ParsePath doesn't handle strings that are not NULL terminated

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9alpha5

People

(Reporter: brettw, Assigned: sciguyryan)

Details

Attachments

(1 file, 2 obsolete files)

The URL parsing code is written such that the input may or may not be NULL terminated. However, the first for loop in nsBaseURLParser::ParsePath uses a NULL for the termination condition, which will cause us to read random memory if the input isn't NULL terminated.

In practice, this code seems to be only be run over the output of PromiseFlatCString which guarantees a NULL terminator. However, this may be a problem in the future, especially since the parsers go pretty far out of their way to allow non-NULL terminated input.
One simple fix here would be to create a new |PromiseFlatCString| string from the character string passes as the argument then re-convert this back too a |const char*| array and use that thus ensuring we always end with a string but that seems inefficient.

I'm not sure if the above method is viable though.
Attached patch patch v1.0 (obsolete) — Splinter Review
Patch v1.0

This is probably nicer than the method above because we don't need to duplicate code there and we simply check against the |pathLen| parameter instead.
Assignee: nobody → sciguyryan
Status: NEW → ASSIGNED
Attachment #261465 - Flags: review?(cbiesinger)
Comment on attachment 261465 [details] [diff] [review]
patch v1.0

p != path + pathLen. did you test this patch?
Attachment #261465 - Flags: review?(cbiesinger) → review-
Attachment #261465 - Flags: review-
Attached patch Patch v1.1 (obsolete) — Splinter Review
Patch v1.1

Oops!
Attachment #261465 - Attachment is obsolete: true
Attachment #261484 - Flags: review?(cbiesinger)
Attachment #261484 - Flags: review?(cbiesinger) → review+
Whiteboard: [checkin needed]
mozilla/netwerk/base/src/nsURLParsers.cpp  1.27
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9alpha4
Please set in-testsuite to either "-" or "?", depending on whether this needs a regression test.
Comment on attachment 261484 [details] [diff] [review]
Patch v1.1

>-    for (p = path; *p; ++p) {
>+    for (p = path; p < path + pathLen; ++p)

This fails to compile, backed out.
Attached patch Patch v1.2Splinter Review
Sorry for the trouble everyone - I missed this one because my build file strangely has the missing |{| already so I'm not sure how its missing from the patch.

I'm going to ask biesi to re-review this patch just to be on the safe side.
Attachment #261484 - Attachment is obsolete: true
Attachment #262798 - Flags: review?(cbiesinger)
Attachment #262798 - Flags: review?(cbiesinger) → review+
Status: RESOLVED → REOPENED
Flags: in-testsuite-
Resolution: FIXED → ---
Whiteboard: [checkin needed]
this should get a testcase, though we don't have a way to create one yet.
Flags: in-testsuite- → in-testsuite?
Re-landed:
mozilla/netwerk/base/src/nsURLParsers.cpp  1.29

Sorry I didn't try to fix the bustage the last time - didn't really have time to experiment on the tinderbox :)
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Target Milestone: mozilla1.9alpha4 → mozilla1.9alpha5
You need to log in before you can comment on or make changes to this bug.