Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

garbage chars in status output on Windows #3

Closed
spike0xff opened this issue May 24, 2012 · 12 comments
Closed

garbage chars in status output on Windows #3

spike0xff opened this issue May 24, 2012 · 12 comments

Comments

@spike0xff
Copy link

Hi Kyle - any idea why I get those three garbage characters after 'WebCapture' in the output copy/pasted below?
The resulting .crx seems to be accepted by Chrome without complaint.
Oddly, I seem to only see these when running buildcrx from the 'dos box', not when it runs from my .bat file.

c:\build\DotTwain\firefox-plugin\build\bin\WebCapture\MinSizeRel>buildcrx WebCapture.zip WebCapture.pem
Bulding a test ZIP file
strcat: WebCapture▄■(.crx
Extension name is WebCapture.zip, and will be WebCapture▄■(.crx
-----BEGIN PUBLIC KEY-----
MIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQC73IWLPvyPexoIWwmKV+Yz2Qti
XfQ0AFKjzyYy7DlWTtsfO6MJXwqt5JpCRSuSFK3/6KaG14k4uJrOe8H1tRLNQnok
AZBdzviF+s8Og8Y6qbA0PDxqjofqIQrQp8uSi6EtSxVqoz04nXyI9OChboGCvRaI
Wh5rcbVoxZtKdxNhaQIDAQAB
-----END PUBLIC KEY-----
Content Size: 1417742 (1384.51 KB)
RSA Keysize: 128, RSA (DER) size: 162
Signature Size: 128
Saved extension to WebCapture▄■(.crx

@kylehuff
Copy link
Owner

Hmm, I'm not sure -- it looks like some type of encoding issue, but I can't seem to reproduce it in my DOS environment.

Given that I can't make it happen, I'm at a loss as to how to correct it. I will continue to look around, but I don't notice anything obvious at the moment.

@m417z
Copy link
Contributor

m417z commented Jan 12, 2014

I don't know how relevant is it after 2 years, but the problem is that the strncpy function doesn't null-terminate the string.
https://github.com/kylehuff/buildcrx/blob/master/buildcrx.c#L44
A workaround is to use the optional third argument.

P.S. Thanks for this nifty tool :)

@kylehuff
Copy link
Owner

@RaMMicHaeL have you experienced this issue? I cannot reproduce the output problem in Windows, Linux or OSX.

I am reluctant to make any changes without being able to verify the effectiveness. Having said that, I see there are some white-space issues (mix of tabs/spaces) -- I'll correct that.

P.S. you are welcome, I hope you find it useful.

@m417z
Copy link
Contributor

m417z commented Jan 12, 2014

@kylehuff, yes, I can reproduce it on my system.
You're probably just being lucky and the string is already filled with zero bytes on your system.
Here's a simple demonstration of the bug:
http://ideone.com/4pT6er

To fix it, just replace this:

    char dst[30];
    strncpy(dst,argv[1],strlen(argv[1])-4);

With:

char dst[30];
int dst_len = strlen(argv[1])-4;
strncpy(dst,argv[1],dst_len);
dst[dst_len] = '\0';

The code is not ideal, either, as having a zip file name longer than 30 characters will cause a buffer overrun.
You might not care too much, but simply bumping the 30 to e.g. 256 should minimize the chances for it.

kylehuff added a commit that referenced this issue Jan 12, 2014

Verified

This commit was signed with the committer’s verified signature. The key has expired.
kylekurz Kyle Kurz
@kylehuff
Copy link
Owner

I seem to remember there was a reason for the char array size of 30 -- but since I can't find a legit reason, I changed the initialization of to be the size of dst_len.

Please let me know if this resolves the issue, as I still cannot reproduce it (for me, the ideone link did not exhibit the issue described)

@m417z
Copy link
Contributor

m417z commented Jan 12, 2014

I changed the initialization of to be the size of dst_len.

That's not correct - dst needs more space than this. In addition to strlen(argv[1])-4, it needs space for ".crx" and for a NULL terminator.
Replace this:

char dst[dst_len];

With:

char dst[dst_len+4+1];

Please let me know if this resolves the issue

Unfortunately, I don't know how to correctly compile the project with Visual Studio.

the ideone link did not exhibit the issue described

Look at the code (exactly what you've had) and the output below. You would expect it to print hello_world, but it prints hello_world!!!!!!!!!!!!!!!!!!. That happens because a NULL terminator is not written. As dst wasn't initialized, it can contain anything. e.g. for the bug reporter, WebCapture▄■(.crx shows that the buffer contained this: ▄■(.

@kylehuff
Copy link
Owner

dst never actually gets the suffix. I just pushed a commit that initializes dst as the entire length of argv[1], and fills it as needed.

@m417z
Copy link
Contributor

m417z commented Jan 13, 2014

On line 47, you're assigning the pointer of dst to filename. On line 56, you're appending the suffix to filename, which is actually pointing to dst.

I saw you've fixed it, but it's still not correct. You didn't account for the NULL terminator. I'll post a pull request.

@kylehuff
Copy link
Owner

Yeah, I was in the middle of taking a test when I made those changes. thanks for the pull request, I'll merge it.

@m417z
Copy link
Contributor

m417z commented Jan 17, 2014

Great.
Now, in order to mark the issue as fixed, you should update the binaries :)

@kylehuff
Copy link
Owner

Closing with the release of v0.2 -- kylehuff/buildcrx/releases/tag/v0.2

@m417z
Copy link
Contributor

m417z commented Feb 8, 2014

I can confirm that it works as expected now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants