506394 - ExpandEnvironmentStringsW in CE shunt isn't quite right

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

As discussed with Vlad an on bug 504385 comment 3, the function we have in the shunt misses % parameters.

In Windows, "ceplayer.exe %1" will be returned as is. "ceplayer.exe %1 %2" will also be returned as is. Our implementation treats the first case as an invalid parameter. I'm not sure about the 2nd case, but it might mess up.

So the logic in our shunt implementation should be fixed to handle these cases better.

Comment 1

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v0.1

This handles the space case & the end of string case. I did a little bit of code cleanup (for consistency's sake) & some whitespace cleanup while I was in there.

I tested against the Windows API (in XP) with the following strings and we now match correctly: "%APPDATA%\ceplayer.exe %1 %", "%APPDATA%\ceplayer.exe %1 %2", "%APPDATA%\ceplayer.exe %1", "ceplayer.exe %1"

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

While the patch addresses the problem, I found that we're still not quite perfect.

Take a basic string "ceplayer.exe" and we're one character short (presumably not accounting for null termination when calculating size on the first pass). Not sure if that's something we want to address now or just spin off or even really care about.

Comment 3

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 391407 [details] [diff] [review]
Patch v0.1

Looks fine to me, though boy is that code super scary.

Comment 4

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Attachment: Patch v1.0 (for check-in)

Taking out a stray variable declaration I accidentally added & minor change to whitespace.

Comment 5

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

pushed http://hg.mozilla.org/mozilla-central/rev/4a39eec28c0a