132949 - Uninitialized pointer used in function call in jsfile.c

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Maynor]

In the function "file_currentDirSetter" the JSObject "rhsObject" is used in 
multiple functions (see below).  Should be initialized to "obj" or 
all "rhsObject" should be changed to "obj".

static JSBool
file_currentDirSetter(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
{
    JSObject *rhsObject; /* should be = obj */
    char     *path;
    JSFile   *file = JS_GetInstancePrivate(cx, rhsObject, &file_class, NULL);

    /* Look at the rhs and extract a file object from it */
    if (JSVAL_IS_OBJECT(*vp)){
        if (JS_InstanceOf(cx, rhsObject, &file_class, NULL)){
...
}

Comment 1

[Phil Schwartau]

Formally confirming for consideration. cc'ing Brendan on this one -

Comment 2

[Brendan Eich [:brendan]]

jsfile.c is not well-supported, but we should get this fix in for 1.0 and js1.5
-- Kenton, can you fix?  Thanks,

/be

Comment 3

[David Maynor]

Attachment: Proposed patch

I made the changes and tested this patch.  There are a few other uninitialized
variables in the code but not as likely to be called or to produce an error
(depends on the compiler).  Let me know if i should open other bug reports.

Thanks
david

Comment 4

[Kenton Hanson (gone)]

Brendan, yes I will follow this.

David, Can you make the patch into a diff file? If not, I can do it.

=Kenton=

Comment 5

[David Maynor]

If there is a diff program (win32) you can point me to sure.  Otherwise I'm out 
of luck -- I'm an old programmer new to open source development :)

david

Comment 6

[Brendan Eich [:brendan]]

That looks like the whole file.  Can you attach a cvs diff -u instead?  Or diff
-u if you're working without cvs for some reason.

Feel free to fix all problems in this bug, no need to split bugs out yet (no
real symptom-based reports yet).  Thanks for doing this!

/be

Comment 7

[Brendan Eich [:brendan]]

Argh, I did it again: I commented based on bugzilla mail notification before
reading all the later comments in the bug.  Anyhoo, you can get cvs for windows
at http://www.cvshome.org/dev/codewindow.html and diff and other tools at
http://sources.redhat.com/cygwin/download.html -- see
http://www.mozilla.org/build/win32.html, where I found these links.

/be

Comment 8

[Phil Schwartau]

Attachment: Proposed patch as a diff file (cvs diff -u against the tip)

Comment 9

[Phil Schwartau]

The patch above is a cvs diff -u of David's fix so far. As Brendan 
said above, if there are any other problems in this file, please feel
free to add fixes for them, too. I can make another diff anytime,
if you attach your jsfile.c to this bug -

Comment 10

[John Bandhauer]

A problem with a file that is not part of the default build of either mozilla or
spidermonkey is not a blocker.

Comment 11

[David Maynor]

Attachment: Proposed patch with all found possibly used unitialized vars

This patch (diff -u and I hope I got it right) contains the original mentioned
pointer and four others.  The others are likely to not cause a problem but I
figured why not fix them them anyway.  oh and you're welcome Brendan --
anytime.

david

Comment 12

[Kenton Hanson (gone)]

Brendan or David, can you advise how to use/test this file?

Thanks,

Kenton

Comment 13

[David Maynor]

To test the patch the following script can be executed using the shell or 
another embedding againt a library compiled with "JS_HAS_FILE_OBJECT" :

f = new File();
f.currentDir = new File("D:\\");

david

Comment 14

[Kenton Hanson (gone)]

Tried David's suggestion on the MAC using Codewarrior with no success.  I
cuoldn't resovle all the links.  I suspect jsfile support is supported only in
the PC environment.  Any comments or suggestions?  I will try the patch using VC++.

Comment 15

[Phil Schwartau]

Also note bug 181324,
"jsfile.c buggy on Windows"

Comment 16

[timeless]

Comment on attachment 75799 [details] [diff] [review]
Proposed patch with all found possibly used unitialized vars

note that in jseng comments should be sentences, so +	  maybe not necessary */
is missing a period "." repeatedly.

Also, CVS is responsible for versioning, so you don't need to include comments
saying who you are and what you did.

Also, freeing a null pointer isn't liked by certain CRTs...

I'm temporarily claiming ownership of this bug. but if i don't fix it within a
month it should be reset to the default owner (my attention span sucks).

phil: i'm slowly working through jsengine bugs, but i haven't seen one about
codewarrior not liking jsfile, do you know of one? (a quick look through the
buglist i'm reading doesn't show one.)
http://www.mozilla.org/js/js-file-object.html indicates that no one has tried to
build it on macos... i'd say that should be covered in another bug :).

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: some cleanup, (re-)write dirname again

I fixed some of the style problems in the functions pointed out in attachment
75799 [details] [diff] [review]. I still need to fix the one reported by e-mail, though. I'll do that
soon.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: fix readln too

Along with the uninitialized pointers and the poorly implemented
js_fileDirectoryName, this fixes file_readln to work for lines greater than 256
characters in length.

Shaver, you get the review request, feel free to take your time getting to
this.

Comment 19

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: more fixes

This patch keeps growing as I find stuff that just needs fixing. Sorry. This
addresses a bunch of comments that Shaver made earlier. Note the changes to
js_fileDirectoryName and file_readln.

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 181324 has been marked as a duplicate of this bug. ***

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 192241 [details] [diff] [review]
more fixes

shaver says on IRC: rs+a=him

Comment 22

[Blake Kaplan (:mrbkap) (inactive)]

I checked attachment 192241 [details] [diff] [review] into the trunk and branch, so I'm marking this bug
FIXED. There are more (probably lots more) bugs (and improvements) to be made in
jsfile.c, but they should be handled in seperate bugs. I'll try to hack on them
when I have time, but it's looking like every other bug on my list has higher
priority. In the meantime, jsfile.c should be relatively safe to use, only
stomping on bad memory in the odd case, instead of every case.