Closed Bug 103468 Opened 20 years ago Closed 18 years ago

Mozilla creates .url files with zero byte at end, confusing IE (Internet shortcut)

Categories

(Core :: DOM: Copy & Paste and Drag & Drop, defect, P1)

x86
Windows NT
defect

Tracking

()

RESOLVED FIXED
Future

People

(Reporter: dolmen, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(7 files, 6 obsolete files)

See also bug #103466 for related information/bug about Internet Shortcuts.

Mozilla always put an \0 as the last byte of Internet Shortcuts.

Microsoft Internet Explorer 6 doesn't seem to be able to load (double-click in
Windows Explorer) the shortcut when last byte of the file is \0 AND the file
length is a multiple of 2.
Of course, the file size depend on the length of the URL bookmarked.

It is probably a Windows UNICODE-related problem as the "type" command or the
notepad also have problems with such files.

I'm attaching below two Internet Shortcuts :
- one that works with IE6/type/notepad (Mozilla 0.9.4 Release Notes (2).URL)
- one that doesn't work (Mozilla 0.9.4 Release Notes.URL)
I forgot to say that I verified that removing the last byte of the file (the
'\0') in the Internet Shortcut solved the problem (IE/notepad/type can load the
shortcut).
-> bookmarks
Assignee: asa → ben
Component: Browser-General → Bookmarks
QA Contact: doronr → claudius
Summary: .url Internet shortcut with zero byte at end → Mozilla creates .url files with zero byte at end, confusing IE (Internet shortcut)
Blocks: 94465
No dupes found. Marking NEW.

-> XP Apps: Drag and Drop

Reporter: In future, please use the Bugzilla Helper for reporting bugs:
http://www.mozilla.org/quality/help/bugzilla-helper.html
(among other things, it automagically adds your Build ID)
Assignee: ben → blakeross
Status: UNCONFIRMED → NEW
Component: Bookmarks → XP Apps: Drag and Drop
Ever confirmed: true
QA Contact: claudius → tpreston
Blocks: 104933
Target Milestone: --- → mozilla1.0
Target Milestone: mozilla1.0 → Future
Verified in 0.9.6 (build 2001112009) that the bug (the '\0' byte) is still there.
Added as an attachment an Internet shortcut created with Mozilla 0.9.6.

However due to the patch for bug #103466 that added a carriage return at the end
of the URL, the bug is now not blocking with IE.

But please, remove this little byte!

Keywords: mozilla0.9.7
Still in Mozilla 0.9.7.
ok, so there's a patch, but this code was obviously very deliberate.  cc'ing
pinkerton for explanation...?
uh, the code that the patch removes does not introduce the null. what it does is
truncate the string so that it's length is <= MAX_PATH. Removing that code would
be bad. 

the right fix would be to trim the null off the result of the sprintf (which is
where the null actually gets introduced). I'm not sure how removing those lines
fixes the problem.
Status: NEW → ASSIGNED
The bug is still in Mozilla 1.0RC1 (build 2002041711).
qa contact -> pmac
QA Contact: tpreston → pmac
There have been recent check-ins in this area.

Worksforme Build ID 2002120708 pre-1.3 alpha, Windows 98.

Reporter, please download and install the latest-trunk build of Mozilla. It
should now work.
The bug still exists in Mozilla 1.6a under Windows 2000. Why?
Attached patch patch to fix the null byte (obsolete) — Splinter Review
Problem was that:
1. PR_snprintf adds a null to the end of the formatted string
2. need to allocate a buffer big enough for the null byte that snprintf adds
3. the entire contents of the buffer are written to disk

Fix is: 
1. only allocate a buffer big enough for the data and no null
2. use memcpy to format the string (easy since it is just a prefix and suffix)
Attachment #148364 - Flags: review?(firefox)
Attached patch patch #2 (obsolete) — Splinter Review
Another patch to fix this bug and more. Bugs addressed with this patch are all
wrapped up in one:
* this bug
* problems where MBCS
Attachment #148364 - Attachment is obsolete: true
Attached file testcases (obsolete) —
Accidently click 'submit' before I was ready.

Fixes are:
* zero byte removed from shortcut file
* code simplified and memory allocations removed
* long titles when converted to MBCS will not be cut in the middle of a
multi-byte character but only at character borders
* the test for an invalid filename includes all of the characters in MSDN
listed as invalid, as well as a test for invalid filenames like "nul"
* all possible error conditions (I think) are handled gracefully

This file is a number of testcases that I created. Contents are:
1. test for invalid characters in name
2. test for invalid filename
3. test for simple MBCS filename (includes backslash as trail-byte)
4. test for very long MBCS filename which exercises the "title too long" case
Attached patch final patch (obsolete) — Splinter Review
I found another couple of possible issues:
* now using the localized string for "untitled page" when a title can't be
generated
* handles a page with no title (fixes bug 104933)
* fixes the problem case of a filename starting with a forbidden string (e.g.
"nul.txt", which in the process of testing IE behaviour ended up with a file on
the desktop called "nul.txt" which I can't delete).
* split the code into separate functions to make it easier to read

I'm definitely finished hacking on this now. 
It'd be nice to get a code review.
Attachment #148732 - Attachment is obsolete: true
Attached file updated testcases (obsolete) —
Attachment #148733 - Attachment is obsolete: true
Attachment #148796 - Flags: review?(firefox)
Attachment #148364 - Flags: review?(firefox)
Attached patch patch 4 (obsolete) — Splinter Review
compress whitespace within the title and trim the ends.
fix a GlobalFree without GlobalUnlock (not strictly necessary but cleaner)
Attachment #148796 - Attachment is obsolete: true
Attachment #149178 - Flags: review?(danm-moz)
Attachment #149178 - Flags: superreview?(ere)
Attachment #149178 - Flags: superreview?(roc)
Attachment #149178 - Flags: superreview?(ere)
Attachment #149178 - Flags: review?(ere)
Attachment #149178 - Flags: review?(danm-moz)
+  static struct { char* name; size_t nameLen; } forbiddenNames[] = {
+    { "CON",  0 }, { "PRN",  0 }, { "AUX",  0 }, { "NUL",  0 }, { "CLOCK$", 0 }, 
+    { "COM1", 0 }, { "COM2", 0 }, { "COM3", 0 }, { "COM4", 0 }, { "COM5", 0 }, 
+    { "COM6", 0 }, { "COM7", 0 }, { "COM8", 0 }, { "COM9", 0 }, { "LPT1", 0 }, 
+    { "LPT2", 0 }, { "LPT3", 0 }, { "LPT4", 0 }, { "LPT5", 0 }, { "LPT6", 0 }, 
+    { "LPT7", 0 }, { "LPT8", 0 }, { "LPT9", 0 }
+  };

I think you should not store the length. This is not performance critical code,
so we should do the simplest thing, which is to just use strlen to get the
length every time.

Other than that, looks fantastic. If you respin a patch I'll give it sr+. If ere
 is not responding (I haven't seen him in IRC, so he may be incommunicado) I'll
stretch out and give this r+ myself.
Comment on attachment 149178 [details] [diff] [review]
patch 4

minusing in anticipation of a new patch
Attachment #149178 - Flags: superreview?(roc) → superreview-
Attached patch patch 5Splinter Review
Patch with requested changes. Also using existing definition from nsCRT.h of
illegal characters.
Attachment #149178 - Attachment is obsolete: true
Attachment #149548 - Flags: superreview?(roc)
Attachment #149548 - Flags: review?(ere)
Attachment #149548 - Flags: superreview?(roc) → superreview+
Comment on attachment 149548 [details] [diff] [review]
patch 5

r=ere
Attachment #149548 - Flags: review?(ere) → review+
Attachment #149178 - Flags: review?(ere)
I don't have CVS write permissions, so would one of you please check this in 
for me? This will fix both this bug and bug 104933.
Taking for checkin
Assignee: firefox → roc
Status: ASSIGNED → NEW
Priority: -- → P1
Checked in. Thank you very much!!
Assignee: roc → nobody
QA Contact: pmac
Checked in. Thank you very much!!
Status: NEW → RESOLVED
Closed: 18 years ago
QA Contact: pmac
Resolution: --- → FIXED
+      strcpy(fileGroupDesc->fgd[0].cFileName, "Untitled.URL"); // fallback
Should this be localizable?
It is. This value is only used if we failed to get the localized string.
Did this get rid of the fix that was originally checked in for bug 78509?  I
can't see that old code in the patch anywhere.
If I understand correctly, this patch just removes all colons from the title
before choosing the file name.
This patch supercedes the patch in bug 78509. Look at the function
MangleTextToValidFilename() for full behaviour. It gets rid of all of the
invalid characters as listed in that bug (FILE_PATH_SEPARATOR 
FILE_ILLEGAL_CHARACTERS) and checks for invalid filenames (nul, lpt1.txt, etc). 

The only thing it doesn't do is strip ascii chars < 32. If these chars should
also be removed then create a new bug and I'll supply a patch.

Note that this resolves bug 104933 and bug 78509 too.
If required, this patch is an addition to the existing checked in one and will
remove all control characters < 32 from the title. I'm not sure that we need
this however, as at least on Windows XP, the OS appears to do this for us.
Attached file testcases
Updated testcases including one for for control characters. test8.html includes
all control chars < 32 in the title. When viewing, ensure that you manually set
the char encoding back to Windows-1252 for correct display. When this file is
dragged from the browser to the desktop on Windows XP, both IE and Moz (1.7RC2)
will correctly create a file using the title with all control chars stripped,
or changed to spaces (for \t\r\n). It seems that the OS is handling this. I
don't know if non-XP versions also do the same processing.
Attachment #148797 - Attachment is obsolete: true
Thanks Brodie.  I searched through the patch repeatedly and couldn't find the
lines that removed the invalid characters.  I'm happy now.
*** Bug 159773 has been marked as a duplicate of this bug. ***
Shift-Jis(Japanese Charactor) contains 0x5c.
Patch of this bug also fix Bug 159773.
If 1.7 becomes stable branck, I want to check in this patch 1.7 branch too.
Too late for 1.7, it's already released. 
Perhaps 1.7.1?
yes 1.7.1 if that will be release.
Is this patch reflected in AVIARY_1_0_20040515_BRANCH?
No. Firefox is bug 247227.
Nominating for fixing in the 1.7 branch for 1.7.1
patch is "patch 5", attachment 149548 [details] [diff] [review]
Flags: blocking1.7.1?
Attachment #149548 - Flags: approval1.7.x?
Attachment #149548 - Flags: approval1.7.5? → approval1.7.5-
Any chance that this will be going into 1.7.5? Is there a reason now for
approval1.7.5- for the patch?
The comment I received from Roc is that this won't be accepted for 1.7.5. The
only branch in which this fix exists is the trunk, so at the moment only Mozilla
1.8a1+ have this fixed. When Firefox 1.1 is released, because it is based on the
trunk it will also get this fix.
Flags: blocking1.7.5?
Depends on: 481126
You need to log in before you can comment on or make changes to this bug.