710993 - Possible bad pointer/size mistake in HTTPUpload::SendRequest()

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[Justin Dolske [:Dolske]]

From http://www.viva64.com/en/a/0078/,
16th section in http://www.viva64.com/external-pictures/txt/mozilla-test.txt

V579 The InternetSetOptionW function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the fourth argument.
http_upload.cc 152

bool HTTPUpload::SendRequest(..., int *timeout, ...)
{
  if (timeout) {
    if (!InternetSetOption(request.get(),
                           INTERNET_OPTION_SEND_TIMEOUT,
                           timeout,
                           sizeof(timeout))) {
      fwprintf(stderr, L"Could not unset send timeout, continuing...\n");
    }
  ...
}

And here:
V579 The InternetSetOptionW function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the fourth argument.  http_upload.cc 159

Comment 1

[(not currently active) Ted Mielczarek]

This is these two calls right here:
http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/common/windows/http_upload.cc#152

they probably just need to be changed to sizeof(*timeout). This code is probably broken on Win64 as it stands.

Comment 2

[Matt Brubeck (:mbrubeck)]

Sushant is interested in working on this bug.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Hi Sushant, do you need any help on how to get the source code or compile Firefox? These two pages have good instructions: https://developer.mozilla.org/En/Developer_Guide/Build_Instructions and https://developer.mozilla.org/En/Developer_Guide/Build_Instructions/Windows_Prerequisites

To fix this bug you'll need to take a look at the API definition for InternetSetOption and verify that we're indeed using the fourth parameter wrong (i.e., that's not a false alarm). And then you can make the change to correct the code and generate a diff using Mercurial.

If you need any advice let me know.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

(These aliases are only used as a shortcut for very popular/important bugs where a big number of people need to access them quickly without looking up the bug number)

Comment 5

[Himanshu]

Attachment: Corrected: Possible bad pointer/size mistake in HTTPUpload::SendRequest()

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 642499 [details] [diff] [review]
Corrected: Possible bad pointer/size mistake in HTTPUpload::SendRequest()

Review of attachment 642499 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the patch Himanshu.

It would be good to track this downstream change within a file in our repository so the next time we pull in upstream changes we can make sure to still include this change. There already exists a file named ChangeLog in toolkit/crashreporter/google-breakpad that could be used for this purpose (I'm not sure what other purpose it serves), or we could create a file named MOZCHANGES (borrowed the naming from /media/libjpeg/MOZCHANGES) that lists our changes. Since I don't see that the ChangeLog file has been used at all before, can you place a reference to this bug and a description of the change in the ChangeLog file?

After getting this bug fixed on our end, we should try to get this bug fixed upstream. To do that, you can follow the steps outlined in the google-breakpad README (http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/README?force=1)

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 642499 [details] [diff] [review]
Corrected: Possible bad pointer/size mistake in HTTPUpload::SendRequest()

This code is owned by Ted, so he'll review the changes. Thanks for the patch!

Comment 8

[Himanshu]

(In reply to Jared Wein [:jaws] from comment #6)
> Comment on attachment 642499 [details] [diff] [review]
> Corrected: Possible bad pointer/size mistake in HTTPUpload::SendRequest()
> 
> Review of attachment 642499 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch Himanshu.
> 
> It would be good to track this downstream change within a file in our
> repository so the next time we pull in upstream changes we can make sure to
> still include this change. There already exists a file named ChangeLog in
> toolkit/crashreporter/google-breakpad that could be used for this purpose
> (I'm not sure what other purpose it serves), or we could create a file named
> MOZCHANGES (borrowed the naming from /media/libjpeg/MOZCHANGES) that lists
> our changes. Since I don't see that the ChangeLog file has been used at all
> before, can you place a reference to this bug and a description of the
> change in the ChangeLog file?
> 
> After getting this bug fixed on our end, we should try to get this bug fixed
> upstream. To do that, you can follow the steps outlined in the
> google-breakpad README
> (http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-
> breakpad/README?force=1)

I could not understand the steps, and how ChangeLog file will get affected by following those steps?

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 642499 [details] [diff] [review]
Corrected: Possible bad pointer/size mistake in HTTPUpload::SendRequest()

Review of attachment 642499 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, thanks! I can land this upstream for you, don't worry about dealing with that. I'll set checkin-needed for you so that someone else will land it in mozilla-central.

Comment 10

[(not currently active) Ted Mielczarek]

Landed upstream:
http://code.google.com/p/google-breakpad/source/detail?r=992

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/68ae3711d782

Thanks for the patch, Himanshu! To make life easier for those checking in on your behalf, please follow the directions below for any future patches you submit. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68ae3711d782