All users were logged out of Bugzilla on October 13th, 2018

NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends.

ASSIGNED
Assigned to
(NeedInfo from)

Status

ASSIGNED
6 years ago
2 months ago

People

(Reporter: ishikawa, Assigned: ishikawa, NeedInfo)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Created attachment 704506 [details] [diff] [review]
A work-in-progress patch for prtime.c

During running the debug build version of TB under valgrind (memcheck),
it was noticed that PR_ParseTimeStringToExplodedTime() was sometimes
scanning the passed buffer well beyond the valid memory range and caused
memcheck to flag the operation as invalid memory access.

After checking the code, it was found that the upperlimit of 1000 is not
quite observed within the subloop in the PR_ParseTimeStringToExplodedTime().

So the stricter checking is inserted.

(The original cause of the problem may be related to the passing of bogus
memory area due to possible memory corruption: 
See Bug 814870 - Invalid read of size 1 in PR_ParseTimeStringToExplodedTime (prtime.c:1450) )

Proposed patch attached.
 
(I have a more detailed patch to clean up some hard to read code, addition of CET (Central European Time) and such,
but I only have the bound-check and removal of trailing blanks in this patch.)
(Assignee)

Updated

6 years ago
Attachment #704506 - Attachment is patch: true
(Assignee)

Updated

6 years ago
Attachment #704506 - Flags: review?(wtc)

Updated

6 years ago
Assignee: nobody → ishikawa
Status: NEW → ASSIGNED
Component: General → NSPR
Product: Core → NSPR
Version: unspecified → other
(Assignee)

Comment 1

6 years ago
Hi,

the patch is being tested locally and on the tryserver.
As far as I can tell, it works as expected.

One nit, though. I thought I would try checking Win build on the tryserver, but
obviously something is very wrong with my setup. It builds clean on TryServer for linux32, and linux64, but for windows build, the patch fails in the first place.
From the manner that every single hunk fails I suspect a line terminator (LF vs CRLF) mismatch. 

But that sounds so strange. 
Does people keep two separate copies of source tree for
LF (for linux, say) vs CRLF files (for windows)?
If so, I am not ready to test win build.

But I thought reporting that linux32, and linux64 worked as expected.
(Assignee)

Updated

6 years ago
Blocks: 806293

Comment 2

6 years ago
(In reply to ISHIKAWA, chiaki from comment #1)
> One nit, though. I thought I would try checking Win build on the tryserver,
> but obviously something is very wrong with my setup. It builds clean on
> TryServer for linux32, and linux64, but for windows build, the patch fails
> in the first place.
> From the manner that every single hunk fails I suspect a line terminator (LF
> vs CRLF) mismatch. 
I've heard that there's a bug in comm-central's Try server, if that's what you were using. Apparently you can work around it by removing the ALWAYS_RUN_CLIENT_PY lines from all of the config/mozconfigs/win32/* and replace the --hg-tool=<path> with --apply-patches (which does not take a path) in the build/client.py-args file.
(Assignee)

Comment 3

6 years ago
(In reply to neil@parkwaycc.co.uk from comment #2)
> (In reply to ISHIKAWA, chiaki from comment #1)
> > One nit, though. I thought I would try checking Win build on the tryserver,
> > but obviously something is very wrong with my setup. It builds clean on
> > TryServer for linux32, and linux64, but for windows build, the patch fails
> > in the first place.
> > From the manner that every single hunk fails I suspect a line terminator (LF
> > vs CRLF) mismatch. 
> I've heard that there's a bug in comm-central's Try server, if that's what
> you were using. Apparently you can work around it by removing the
> ALWAYS_RUN_CLIENT_PY lines from all of the config/mozconfigs/win32/* and
> replace the --hg-tool=<path> with --apply-patches (which does not take a
> path) in the build/client.py-args file.

Yes, I have used comm-central's Try Server.
I had this "--apply-paches" line (the second half of your suggestion.)

But I did not remove "ALWAYS_RUN_CLIENT_PY" lines from the config/mozconifgs/win32/* (the first half of your suggestion.)
I will investigate.

TIA
(Assignee)

Comment 4

5 years ago
I think it is about time that this patch go in the tree.
I have been using this locally for a long time.
(Maybe I may have to upload the recent version to make sure the patch applies cleanly.)
I thought that there was an issue of pending patch or something (or some other patches that
may need baking so to speak: see
Bug 814870 - Invalid read of size 1 in PR_ParseTimeStringToExplodedTime (prtime.c:1450)
)
But it has been a year, and so I think I will upload a new patch very shortly to make sure it applies cleanly
and hope it gets the approval for inclusion soon.

TIA
(Assignee)

Comment 5

2 years ago
I will upload a new patch just to make sure that bitrot issue is taken care of.
Flags: needinfo?(ishikawa)
(Assignee)

Comment 6

2 years ago
Created attachment 8796834 [details] [diff] [review]
NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends.

This is an updated patch, and lands cleanly.
I modified the definition of check macro by using |do { ... } while (0)| 
instead of |if (1) { ... } else| since the latter causes GCC compiler warning about surrounding the statement after |else|, etc.

TIA
Attachment #704506 - Attachment is obsolete: true
Attachment #704506 - Flags: review?(wtc)
Flags: needinfo?(ishikawa)
Attachment #8796834 - Flags: review?(wtc)
(Assignee)

Updated

2 years ago
Attachment #8796834 - Flags: review?(wtc) → review?(ted)
(Assignee)

Comment 7

2 years ago
I was not sure if wtc@google.com is still around, and so asked ted to review this.
TIA
(Assignee)

Comment 8

2 years ago
I think wtc@google.com is still around and so request information.
Flags: needinfo?(wtc)
Comment on attachment 8796834 [details] [diff] [review]
NSPR: stricter bound check for prtime.c PR_ParseTimeString and friends.

I have resigned as NSPR module owner. Sorry for the inconvenience.
Attachment #8796834 - Flags: review?(ted)
Chiaki, how about instead advancing rest through a method which would check limits before advancing?

Also, to ease review, drop all the unrelated whitespace changes and such (can be submitted separately), and remove the debugging code like dumpstring.

After updating the patch ask for review - https://wiki.mozilla.org/Modules/All#NSPR
You need to log in before you can comment on or make changes to this bug.