1172304 - A M-C fix to handle short read in Plugin code ( from [META] Failure to deal with short read)

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Handle short-read in ReadPluginInfo().

Hi,

This a fix to handle short-read gracefully in ReadPluginInfo().

This blocks 
Bug 1170564 - [META] Failure to deal with short read

TIA

Comment 1

[ISHIKAWA, Chiaki]

Comment on attachment 8616434 [details] [diff] [review]
Handle short-read in ReadPluginInfo().

I request John who reviewed the issue of failing to check PR_Close() return values near by a couple of years ago.

TIA

Comment 2

[John Schoenick [:johns]]

This is a pretty simple fix, but I wonder if we want to add a read-until-completion-or-error helper to a more global spot as part of 1170564? It seems like we should be having this issue at a sizable chunk of PR_Read callsites

Comment 3

[ISHIKAWA, Chiaki]

(In reply to John Schoenick [:johns] from comment #2)
> This is a pretty simple fix, but I wonder if we want to add a
> read-until-completion-or-error helper to a more global spot as part of
> 1170564? 

Thank you for your comment.

Maybe that is the best course. But I wonder where the routine ought to go in
and where the prototype should be declared.

> It seems like we should be having this issue at a sizable chunk of
> PR_Read callsites

It is possible that there are other call sites that are not protected against short-read.

I found there are two types of routines that call |PR_Read| (or for that matter |Read|).

One type calls |PR_Read| in a loop and asks for data, and
works with whatever data returned, and
continue read and do stuff if the available in-memory data is exhausted.
If either the end of the file or the logical end of data is reached, the loop is exited.

This first type does not suffer from premature short read.

The second type calls |PR_Read| by checking the expected amount to read in advance
and tries to read that amount in one |PR_Read| (in a few cases a |stat| is done against a file to know the file's size and read the whole file in one go.)

The second type suffers from short-read.

There are actually a mixture of first and second types and
so such function are hard to deal with.
(|NS_ReadLine| is such a beast.) 

I have been posting fixes and file bugizilla entries ONLY for
places where I verified that short-read cause problems 
(and C-C TB forgets to read the remaining
octets which under normal circumstances would have read)
by tracing read calls after injecting short-read from my PRELOADED |read| routines.

The testing is only from running C-C TB through |make mozmill| test suite only, AND
the simulated short-read injection was done only against a selected set of files under a test
profile directory including Mail store directory, and /tmp directory and such. 

If I expand this set of files, I may find more cases of |PR_Read| not checking
for short-read. But I can't tackle all the problems at the same time.

BTW, this manifests very badly under linux.
The big most offender, large copy of file, is handled by a single win32 system call
under Windows and it seems that the win32 system call internally handles short-read
and other issues under the hood. For linux, it is handled by an open-coded CPP loop.

But I am not sure if C-C TB does not suffer from short read under windows at all for other cases.
Obviously, some short read fixes I posted seem to affect Windows version as well. But I may have overlooked something. 

TIA
 
TIA

Comment 4

[Andrew McCreight [:mccr8]]

Maybe you have some kind of opinion on this PR_Read issue, Nathan?

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Maybe you have some kind of opinion on this PR_Read issue, Nathan?

I think it'd be helpful to have some sort of wrapper around PR_Read or similar.  I'm not sure what a good interface for the wrapper would look like; almost anything you put together is going to be clunkier than PR_Read.  You also have to convince people to use it...

Comment 6

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> (In reply to Andrew McCreight [:mccr8] from comment #4)
> > Maybe you have some kind of opinion on this PR_Read issue, Nathan?
> 
> I think it'd be helpful to have some sort of wrapper around PR_Read or
> similar.  I'm not sure what a good interface for the wrapper would look
> like; almost anything you put together is going to be clunkier than PR_Read.
> You also have to convince people to use it...

Well there's already a proposed replacement in attachment 8616434 [details] [diff] [review] and it has the exact same interface. It would be nice if this were the default behavior for |PR_Read|, but that seems like a somewhat larger change. We'd probably want to handle EINTR as well (as noted in the patch), but that gets you into an area where you'd want to have a timeout.

What's more interesting is this type of operation would be useful for any type of read operation (think tcp recv, pipes, etc), I'm not sure how doable it is to make it multipurpose in the context of NSPR though.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Eric Rahm [:erahm] from comment #6)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #5)
> > (In reply to Andrew McCreight [:mccr8] from comment #4)
> > > Maybe you have some kind of opinion on this PR_Read issue, Nathan?
> > 
> > I think it'd be helpful to have some sort of wrapper around PR_Read or
> > similar.  I'm not sure what a good interface for the wrapper would look
> > like; almost anything you put together is going to be clunkier than PR_Read.
> > You also have to convince people to use it...
> 
> Well there's already a proposed replacement in attachment 8616434 [details] [diff] [review]
> [diff] [review] and it has the exact same interface. It would be nice if
> this were the default behavior for |PR_Read|, but that seems like a somewhat
> larger change. We'd probably want to handle EINTR as well (as noted in the
> patch), but that gets you into an area where you'd want to have a timeout.

Right.  That proposed replacement can still result in short reads, though, without any indication to the caller.  It might fix some cases, but you still have to check for short reads, so...

Lack of error handling/EINTR/timeouts are also a problem.  And once you add some or all of those, you get something that's kind of unwieldy to call.

Comment 8

[ISHIKAWA, Chiaki]

Thank you for the feedback.

The replacement wrapper proposed here does not hope to fix everything (that will be the holy grail).
It works well for this case, AND
the short read which could not be fixed (due to real hardware error, etc.)
*IS* checked in this case by the code below

  // short read error, so to speak.
     if (flen > bread)
        return rv;

(We may want to return a better error code though.)

I am approaching this short-read issue
only by modifying the places where
my test harness uncovers the failure to handle short-read at all.

My idea is try to fix only the base minimum and not touch anything else
much if I can afford to.
Mozilla code base has been in existence for so many years and any gratuitous change 
may break something.

I hope this explains my standpoint regarding how to approach the failure to handle short-read.

Yes, it would be nice to PR_Read() handles this automagically, but
it was designed and coded before network file system has become so popular even for ordinary PCs used in 
enterprise setting.

Further comments welcome. After all, it may be better of to move the wrapper to a 
commonly referenced file so that it does not have to be replicated in every file where such
wrapper is missing. But to be honest, I have no idea where it should be moved or , to be frank,
not so convinced if such a universal wrapper is desirable, and better off with a wrapper defined with static file scope (simply to avoid what the wrapper ought to be with EINTR processing, etc., which may not be appropriate in some cases, even?)

TIA

Comment 9

[John Schoenick [:johns]]

Comment on attachment 8616434 [details] [diff] [review]
Handle short-read in ReadPluginInfo().

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

Well, whether or not someone wants to do the work to improve PR_Read in the future, this looks safe enough to take here.

Thanks for digging into this!

Comment 10

[ISHIKAWA, Chiaki]

(In reply to John Schoenick [:johns] from comment #9)
> Comment on attachment 8616434 [details] [diff] [review]
> Handle short-read in ReadPluginInfo().
> 
> Review of attachment 8616434 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Well, whether or not someone wants to do the work to improve PR_Read in the
> future, this looks safe enough to take here.
> 
> Thanks for digging into this!

Thank you. I will put checkin-needed keyword.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Can we please run this through Try? :)

Comment 12

[ISHIKAWA, Chiaki]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Can we please run this through Try? :)

Ok, I have been running this patch on local PC.

But I am afraid that I have a bunch of unrelated patches already accumulating in my queue and it is difficult to get rid of them now.

Can somebody run it on try?

If nobody steps forward, I will try to clear my queue, but it takes a day or two.

TIA

Comment 13

[Stephen A Pohl [:spohl]]

(In reply to ISHIKAWA, Chiaki from comment #12)
> If nobody steps forward, I will try to clear my queue, but it takes a day or
> two.

You might prefer cloning m-c a second time for the sole purpose of pushing to try. I keep a second copy around for this very purpose.

Comment 14

[ISHIKAWA, Chiaki]

(In reply to Stephen A Pohl [:spohl] from comment #13)
> (In reply to ISHIKAWA, Chiaki from comment #12)
> > If nobody steps forward, I will try to clear my queue, but it takes a day or
> > two.
> 
> You might prefer cloning m-c a second time for the sole purpose of pushing
> to try. I keep a second copy around for this very purpose.

Do you mean creating a m-c tree, i.e., having the exact copy from the mozilla repository? It seems easiler albeit needing to keep a somewhat large disk space. 
I am using a virtual PC environment and wanted to avoid attaching more disk images to this environment and was trying to find a neat way by shuffling hg patch queues.
But I think shuffling queue was too error prone.
I may want to follow your suggestion.
Thank you for your tips.

Comment 15

[ISHIKAWA, Chiaki]

Pushed to C-C tryserver: (well, I noticed the issue in C-C TB.)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=fac7b8e09b56

Comment 16

[Stephen A Pohl [:spohl]]

(In reply to ISHIKAWA, Chiaki from comment #14)
> (In reply to Stephen A Pohl [:spohl] from comment #13)
> > (In reply to ISHIKAWA, Chiaki from comment #12)
> > > If nobody steps forward, I will try to clear my queue, but it takes a day or
> > > two.
> > 
> > You might prefer cloning m-c a second time for the sole purpose of pushing
> > to try. I keep a second copy around for this very purpose.
> 
> Do you mean creating a m-c tree, i.e., having the exact copy from the
> mozilla repository? It seems easiler albeit needing to keep a somewhat large
> disk space.

Yes, this is what I meant. You're right that it requires more disk space. Personally, I found it to be worth it as it keeps my error rate much lower when it comes to pushing to try (since I'm not accidentally pushing unrelated patches at the same time). But something that works for me doesn't have to be the best solution for everyone. Just thought I'd throw it out there in case it would help. :-)

Comment 17

[ISHIKAWA, Chiaki]

(In reply to ISHIKAWA, Chiaki from comment #15)
> Pushed to C-C tryserver: (well, I noticed the issue in C-C TB.)
> 
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=fac7b8e09b56

It failed due to some configuration issues, I am afraid. I will re-check after refreshing my local source tree and how it would go the next time. :-(

Comment 18

[ISHIKAWA, Chiaki]

(In reply to Stephen A Pohl [:spohl] from comment #16)
> (In reply to ISHIKAWA, Chiaki from comment #14)
 [...]
> Yes, this is what I meant. You're right that it requires more disk space.
> Personally, I found it to be worth it as it keeps my error rate much lower
> when it comes to pushing to try (since I'm not accidentally pushing
> unrelated patches at the same time). But something that works for me doesn't
> have to be the best solution for everyone. Just thought I'd throw it out
> there in case it would help. :-)

Well, I tried to hack my script to use switching multiple queues (of patches using
hg qqueue extension).
However, it looks it may be error-prone, and am not sure if it is worthwhile to go through the complexity and I suppose just keeping a new repository for testing patches may be a good idea (even for a single patch).
Thank you again.

Comment 19

[ISHIKAWA, Chiaki]

Attachment: Minor tweak for windows compilation Handle short-read in ReadPluginInfo(). Carrying over john= r+

Sorry the patch needed a minor tweak for windows compilation.
ssize_t type is not good for Windows compialtion and PR32Int is now used instead.

TIA

Comment 20

[ISHIKAWA, Chiaki]

Pushed to C-C TB.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7983cac21e61


If C-C TB fails my window compilation for no apparent reason, I will submit
firefox tryserver job. That should be satisfactory to check the 
sanity of the patch.

TIA

Comment 21

[ISHIKAWA, Chiaki]

Hi,

I could finally get a try job for win32 succeed.
(I don't know why but there are server errors obviously this week on top of an issue with win32 job submission.)

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e0d1e9cd52fc

I don't believe my patch introduced any new problems.

I will put checkin-needed keyword.

TIA

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/673fd612aea1

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/673fd612aea1