Bug 129681 - "cvs up" segfaults on corrupt repository file
Summary: "cvs up" segfaults on corrupt repository file
Status: RESOLVED FIXED
Alias: None
Product: SUSE LINUX 10.0
Classification: openSUSE
Component: Development (show other bugs)
Version: RC 4
Hardware: All SuSE Linux 10.0
: P5 - None : Normal
Target Milestone: ---
Assignee: Olaf Dabrunz
QA Contact: E-mail List
URL: http://savannah.nongnu.org/bugs/index...
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-10-20 11:24 UTC by Olaf Dabrunz
Modified: 2005-10-21 11:00 UTC (History)
0 users

See Also:
Found By: Development
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments
Patch adding a sanity check to RCS_parsercsfile_i() guarding against corrupted repository files (2.20 KB, patch)
2005-10-20 11:25 UTC, Olaf Dabrunz
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Olaf Dabrunz 2005-10-20 11:24:07 UTC
The corrupt repository file contains no "head" key (the first 4039 bytes were
overwritten by zeroes).

cvs up is called.

RCS_parsercsfile_i() does not find a "head" key and simply leaves NULL in the
"head" field of the RCSNode.

Later, the checked out revision is compared against the head revision.
RCS_checkout() is called to get the head revision from the repository file and
segfaults on

    if (rev == NULL || STREQ (rev, rcs->head))

because rcs->head is NULL.

Discussion:

It would be good if RCS_parsercsfile_i() could report an error on corrupt
repository files.

But RCS_parsercsfile_i() is supposed to only do a quick scan of the repository
file's keys for HEAD, BRANCH, and EXPAND keys. Knowledge of the complete
structure of a repository file is in RCS_reparsercsfile(). To keep this
maintainable, RCS_parsercsfile_i() should not duplicate this knowledge.

Maybe the best compromise is to add a simple and quick heuristic that reports
an error for a set of corruptions, e.g. if the key is too long.

I intended to post a patch here, but then I found that rcsbuf_getkey() may call
realloc(3) (via rcsbuf_fill()) between assigning the buffer pointer to *keyp
and finding the end of the key. This is a bug in itself (because realloc(3) may
move the buffer), which requires to change the handling of the buffers (i.e.
the offsets must be remembered instead of the pointers). But rather than going
on a bug-fixing trip that may result in a (minor?) redesign, I would like to
have a workable fix for the initial bug.

Therefore it may be better to catch only the "overwritten with zeroes"
corruption (and similar). rcsfile(5) effectively says that keys are limited to
"any visible graphic character except special ($ | , | . | : | ; | @)". So it
may be a good heuristic to outlaw all control characters in keys (except for
the whitespace characters, which end a key).

Patch is attached.

I submitted the same bug and patch upstream today:

http://savannah.nongnu.org/bugs/index.php?func=detailitem&item_id=14830
Comment 1 Olaf Dabrunz 2005-10-20 11:25:54 UTC
Created attachment 54912 [details]
Patch adding a sanity check to RCS_parsercsfile_i() guarding against corrupted repository files
Comment 2 Olaf Dabrunz 2005-10-20 11:31:10 UTC
Updated time estimate: it took about 4 hours to build a debug package, debug
this, analyze and discuss, create a patch and submit to 2 Bug-Trackers (here
and upstram).
Comment 3 Olaf Dabrunz 2005-10-21 10:59:39 UTC
Submitted patch to STABLE.
Comment 4 Olaf Dabrunz 2005-10-21 11:00:16 UTC
Fixed.