Hi, This a fix to handle short-read gracefully in ReadPluginInfo(). This blocks Bug 1170564 - [META] Failure to deal with short read TIA

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

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

(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

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

(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...

(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.

(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.

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 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!

(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.

Can we please run this through Try? :)

(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

(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.

(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.

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

(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. :-)

(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. :-(

(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.

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

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

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